Refactor Viewport and SimulatedRegion position to be the center instead of a corner #675

Dassderdie · 2023-02-16T15:54:13Z

Refactor Viewport and SimulatedRegion position to be the center instead of a corner
Simplified GeometryHelper API by removing Positions<>
The ResizeRectangleInteraction now has a minimum size and prevents flipping of the rectangle

…ad of a corner * Simplified GeometryHelper API by removing `Positions` * The ResizeRectangleInteraction now has a minimumSize and preventsflipping of the rectangle

.../exercises/exercise/shared/exercise-map/feature-managers/simulated-region-feature-manager.ts

benn02

LGTM but it still has unresolved conflicts, please resolve them for approval

Greenscreen23 · 2023-02-28T11:46:11Z

...src/app/pages/exercises/exercise/shared/exercise-map/utility/resize-rectangle-interaction.ts

+        const oldXLength = draggedCorner - originCorner;
+        const newXLength = mouseCoordinate - originCorner;


Please remove the X from those variable names, as it is also used for Y coordinates.

Greenscreen23 · 2023-03-01T09:56:29Z

shared/src/state-migrations/21-refactor-rectangular-element-positions-to-center.ts

+                case '[Viewport] Add viewport':
+                    if (action.viewport.position.type === 'coordinates') {
+                        oldViewportSizes[action.viewport.id] = cloneDeep(
+                            action.viewport.size
+                        );
+                    }
+                    migrateRectangularElement(action.viewport);
+                    break;
+                case '[Viewport] Move viewport': {
+                    const oldViewportSize = oldViewportSizes[action.viewportId];
+                    if (oldViewportSize) {
+                        migratePosition(action.targetPosition, oldViewportSize);
+                    }
+                    break;
+                }
+                case '[Viewport] Resize viewport': {
+                    const oldViewportSize = cloneDeep(action.newSize);
+                    oldViewportSizes[action.viewportId] = oldViewportSize;
+                    migratePosition(action.targetPosition, oldViewportSize);
+                    migrateSize(action.newSize);
+                    break;
+                }


What about actions related to simulated regions?

Added them.

lukasrad02

Besides the aspects mentioned by Benildur and Lukas, I'm fine with this.

…sition-from-corner-to-center

Dassderdie · 2023-03-18T15:05:51Z

shared/src/state-migrations/25-refactor-rectangular-element-positions-to-center.ts

+/**
+ * Migrates the position of a rectangular element to the center of the element.
+ *
+ * This migration is not accurate for previously "flipped" viewports (height and/or width was negative).
+ * To catch this case, access to the size of the viewport before this migration would be required.
+ *
+ * @param migratedSize the size of the element after it had been migrated to the center
+ */
+function migratePositionWithMigratedSize(coordinates: any, migratedSize: any) {
+    migratePosition(coordinates, migratedSize);
+}


The move-actions only include the targetPosition. However, the "real" position of the element was defined by its position and size (because of negative height/width). This information gets lost during this migration. Therefore, the current migration system doesn't allow for an accurate migration in these cases.

I believe this to be acceptable, as this is an edge case that would only result in some regions in imported exercise histories being at slightly different positions.

Depending on the position and size, the location can differ a lot, even with values that are realistic to occur in an exercise. Since resizing of viewports and simulated regions is an operation of exercise creation, this is a breaking change according to our definition (see https://github.com/hpi-sam/digital-fuesim-manv#versions).

We though about a solution for this problem in our team. Our idea was to keep a log which "optical" corner of the rectangle the current location refers to. This log could be updated from the newSize of each resize action before migrating it (so one can deduct the corner from the sign of the width and height). With the information about the corner, one could restore the exact desired position and extent on the map after a move action.

What do you think about this?

Depending on the position and size, the location can differ a lot, even with values that are realistic to occur in an exercise. Since resizing of viewports and simulated regions is an operation of exercise creation, this is a breaking change according to our definition (see https://github.com/hpi-sam/digital-fuesim-manv#versions).

This should only be a breaking change for exercises with history. With my current implementation, states should be losslessly converted to the new definition. Only certain Move-Actions in the history should pose problems.

We though about a solution for this problem in our team. Our idea was to keep a log which "optical" corner of the rectangle the current location refers to. This log could be updated from the newSize of each resize action before migrating it (so one can deduct the corner from the sign of the width and height). With the information about the corner, one could restore the exact desired position and extent on the map after a move action.

What do you think about this?

If I understand it correctly, this would be similar to my previous approach (look at the migration in this PR from a few weeks ago), in which I kept a log of all old viewport sizes. (Though, I believe I forgot to populate the log with the previous sizes of viewports which didn't have a resize or create action.) To still do this, one would have to adjust the migration system again, to realise this log.

Alternatively one could create two migrations for this change and use the first migration to write this log in the intermediate exerciseState and perform the lossless migration. The second one would only remove this log from the state again.

Depending on the position and size, the location can differ a lot, even with values that are realistic to occur in an exercise. Since resizing of viewports and simulated regions is an operation of exercise creation, this is a breaking change according to our definition (see https://github.com/hpi-sam/digital-fuesim-manv#versions).

This should only be a breaking change for exercises with history. With my current implementation, states should be losslessly converted to the new definition. Only certain Move-Actions in the history should pose problems.

At the moment, our definition only differentiates between prepared and played exercises. But prepared exercises can also be exported with history which I think is a legitimate choice.

We though about a solution for this problem in our team. Our idea was to keep a log which "optical" corner of the rectangle the current location refers to. This log could be updated from the newSize of each resize action before migrating it (so one can deduct the corner from the sign of the width and height). With the information about the corner, one could restore the exact desired position and extent on the map after a move action.
What do you think about this?

If I understand it correctly, this would be similar to my previous approach (look at the migration in this PR from a few weeks ago), in which I kept a log of all old viewport sizes. [...]

Yes, this is what we though of

To still do this, one would have to adjust the migration system again, to realise this log.

Alternatively one could create two migrations for this change and use the first migration to write this log in the intermediate exerciseState and perform the lossless migration. The second one would only remove this log from the state again.

Probably not the cleanest solution, but the log could also be saved in a local variable in your typescript file, requiring no changes to the migration system

At the moment, our definition only differentiates between prepared and played exercises. But prepared exercises can also be exported with history, which I think is a legitimate choice.

What is the purpose of not yet played exports with a history? Do you want to keep the option to undo and redo changes in the prepared exercise?

Probably not the cleanest solution, but the log could also be saved in a local variable in your typescript file, requiring no changes to the migration system

This would introduce strange side effects to this particular migration function.
You would have to manually empty the log if the exerciseId of the intermediaryState changes. In addition, you would have to rely on the call of the state-migration function to be directly followed by all the action migration calls of the same exercise (first migrating the initial states of all exercises, and then all their actions would result in incorrect migrations). Otherwise, you would have to save all logs for all exercises, which would be a memory leak.
I would rather opt for my approach as this state introduction could set a bad precedent for future programmers.

What is the purpose of not yet played exports with a history? Do you want to keep the option to undo and redo changes in the prepared exercise?

Yes, this is what I've though of. Trainers might have complex scenarios and then just omit the last couple of actions to create an easier version of them. (Although I think there is no option to do this from the user interface yet).

Another reason for me is that I would expect some people to be in doubt of the meaning of the export options "Kompletter Verlauf" (full history) and "Aktueller Zustand" (current state) and export their prepared exercises as full history just to be safe.

I would rather opt for my approach as this state introduction could set a bad precedent for future programmers.

Are you talking about the two migrations or about the changes to the migration system?

Yes, this is what I've thought of. Trainers might have complex scenarios and then just omit the last couple of actions to create an easier version of them. (Although I think there is no option to do this from the user interface yet).

No, there is no interface for this yet. This could also get a bit tricky with the UX, as you would often not see any changes if you undo something globally (e.g., changing a vehicle name that isn't visible).

Another reason for me is that I would expect some people to be in doubt of the meaning of the export options "Kompletter Verlauf" (full history) and "Aktueller Zustand" (current state) and export their prepared exercises as full history just to be safe.

One could also disallow exporting the history if the exercise hasn't been started yet.

But I'm ok with keeping the current definition and postponing any decisions about this indefinitely for now due to low ROI ^^

Are you talking about the two migrations or about the changes to the migration system?

The two migrations.

A new migration system would need more discussion and would probably also only make sense after we have more experience with the needs of migrations. The last refactoring removed some capabilities (adding actions, combining actions, reordering actions, in general, more context-aware operations on actions). Though I believe all except adding actions can be worked around via two consecutive migrations.

The two migrations.

Okay, sounds good.

A new migration system would need more discussion and would probably also only make sense after we have more experience with the needs of migrations. The last refactoring removed some capabilities (adding actions, combining actions, reordering actions, in general, more context-aware operations on actions). Though I believe all except adding actions can be worked around via two consecutive migrations.

@Nils1729 might be the right person if you'd like to go into the discussion on the migration system (not for this PR but in general)

Refactor Viewport and SimulatedRegion position to be the center inste…

7e1537d

…ad of a corner * Simplified GeometryHelper API by removing `Positions` * The ResizeRectangleInteraction now has a minimumSize and preventsflipping of the rectangle

Dassderdie linked an issue Feb 16, 2023 that may be closed by this pull request

Refactor Viewport and SimulatedRegion position to be the center instead of a corner #666

Open

Dassderdie commented Feb 16, 2023

View reviewed changes

.../exercises/exercise/shared/exercise-map/feature-managers/simulated-region-feature-manager.ts Show resolved Hide resolved

Fix lint errors

cf3db98

Dassderdie self-assigned this Feb 16, 2023

Dassderdie added 2 commits February 16, 2023 23:24

Add migration

7c9e20f

Improve the migration

2b373dc

Dassderdie marked this pull request as ready for review February 16, 2023 22:50

Dassderdie requested review from lukasrad02 and Greenscreen23 February 16, 2023 22:53

Fix lint error

a98b373

Dassderdie enabled auto-merge (squash) February 16, 2023 22:55

benn02 reviewed Feb 28, 2023

View reviewed changes

Greenscreen23 requested changes Feb 28, 2023

View reviewed changes

Greenscreen23 reviewed Mar 1, 2023

View reviewed changes

lukasrad02 reviewed Mar 1, 2023

View reviewed changes

lukasrad02 mentioned this pull request Mar 7, 2023

Show connecting lines for transfers from/to simulated regions #727

Merged

Dassderdie added 5 commits March 18, 2023 14:45

Merge remote-tracking branch 'origin/dev' into bugfix/666-refactor-po…

3901a33

…sition-from-corner-to-center

Update migration function

1c5a392

Rename migration

2d64f36

Rename variable

7d51a52

Update changelog

e0c3aaa

Dassderdie commented Mar 18, 2023

View reviewed changes

Dassderdie requested review from Greenscreen23, lukasrad02 and benn02 March 18, 2023 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Viewport and SimulatedRegion position to be the center instead of a corner #675

Refactor Viewport and SimulatedRegion position to be the center instead of a corner #675

Dassderdie commented Feb 16, 2023 •

edited

Loading

benn02 left a comment

Greenscreen23 Feb 28, 2023 •

edited by anonym-HPI

Loading

Dassderdie Mar 18, 2023

Greenscreen23 Mar 1, 2023

Dassderdie Mar 18, 2023

lukasrad02 left a comment

Dassderdie Mar 18, 2023

lukasrad02 Mar 20, 2023

Dassderdie Mar 20, 2023

lukasrad02 Mar 21, 2023

Dassderdie Mar 21, 2023

lukasrad02 Mar 21, 2023

Dassderdie Mar 21, 2023

lukasrad02 Mar 21, 2023

		const oldXLength = draggedCorner - originCorner;
		const newXLength = mouseCoordinate - originCorner;

Refactor Viewport and SimulatedRegion position to be the center instead of a corner #675

Are you sure you want to change the base?

Refactor Viewport and SimulatedRegion position to be the center instead of a corner #675

Conversation

Dassderdie commented Feb 16, 2023 • edited Loading

benn02 left a comment

Choose a reason for hiding this comment

Greenscreen23 Feb 28, 2023 • edited by anonym-HPI Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasrad02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dassderdie commented Feb 16, 2023 •

edited

Loading

Greenscreen23 Feb 28, 2023 •

edited by anonym-HPI

Loading