Undo create/remove entity, add/remove component #789

vincentfretin · 2024-08-26T13:34:39Z

This closes #704

Lots of refactoring here.
Although I manually tested all that, please do some tests on your own to be sure I didn't break anything and all is working correctly.

…t-geo

…used addEntity

…mand

…ame in delete confirm dialog

…remove entity

vincentfretin · 2024-08-27T11:15:31Z

I asked Claude Sonnet 3.5 on the code base via Cursor IDE.

Prompt

Events are emitted with Events.emit that triggers an action that does a DOM change (that does createElements or setAttribute for eample) and other events are emitted with Events.emit to notify of a change. Do a thorough search of the code base to list them all to write a exhaustive documentation. For each event tell me if it's to create a change or to notify a change. If it's an action and is not currently handled via an undoable command, please tell me. I'm not interested in events emitted with sceneEl.emit or el.emit, only list those emitted with Events.emit.
Please be exhaustive, here is a list that may help you:

grep -r Events.emit src/
src/editor/index.js:    Events.emit('objectremove', object);
src/editor/index.js:        Events.emit('helperremove', this.helpers[node.uuid]);
src/editor/index.js:      Events.emit('entityselect', entity);
src/editor/index.js:    Events.emit('objectselect', object3D);
src/editor/index.js:    Events.emit('inspectortoggle', true);
src/editor/index.js:      Events.emit('objectfocus', focusEl.object3D);
src/editor/index.js:    Events.emit('inspectortoggle', false);
src/editor/components/viewport/CameraToolbar/CameraToolbar.component.js:    Events.emit(option.event, option.payload);
src/editor/components/viewport/TransformToolbar.js:    Events.emit(
src/editor/components/viewport/TransformToolbar.js:    Events.emit('transformmodechange', mode);
src/editor/components/viewport/TransformToolbar.js:    Events.emit('transformspacechanged', local ? 'local' : 'world');
src/editor/components/widgets/TextureWidget.js:    Events.emit('opentexturesmodal', this.state.value, (image) => {
src/editor/components/scenegraph/Entity.js:  onDoubleClick = () => Events.emit('objectfocus', this.props.entity.object3D);
src/editor/components/scenegraph/Toolbar.js:    Events.emit('updatescenegraph');
src/editor/components/scenegraph/Toolbar.js:        Events.emit('opensigninmodal');
src/editor/components/scenegraph/Toolbar.js:        Events.emit('openpaymentmodal');
src/editor/components/scenegraph/Toolbar.js:      Events.emit('opensigninmodal');
src/editor/components/scenegraph/Toolbar.js:      Events.emit('sceneplayingtoggle', false);
src/editor/components/scenegraph/Toolbar.js:    Events.emit('sceneplayingtoggle', true);
src/editor/components/scenegraph/Toolbar.js:              onClick={() => Events.emit('openscenesmodal')}
src/editor/components/scenegraph/Toolbar.js:              Events.emit('openscreenshotmodal');
src/editor/components/scenegraph/SceneGraph.js:        Events.emit('entityselect', entity);
src/editor/components/Main.js:            Events.emit('togglesidebar', { which: 'attributes' });
src/editor/components/Main.js:            Events.emit('togglesidebar', { which: 'scenegraph' });
src/editor/components/components/ProfileButton/ProfileButton.component.jsx:      return Events.emit('openprofilemodal');
src/editor/components/components/ProfileButton/ProfileButton.component.jsx:    return Events.emit('opensigninmodal');
src/editor/components/components/GeoPanel/GeoPanel.component.jsx:      Events.emit('opensigninmodal');
src/editor/components/components/GeoPanel/GeoPanel.component.jsx:      Events.emit('opengeomodal');
src/editor/components/components/GeoPanel/GeoPanel.component.jsx:      Events.emit('openpaymentmodal');
src/editor/components/components/Mixins.js:    Events.emit('entityupdate', {
src/editor/components/components/Mixins.js:    Events.emit('entityupdate', {
src/editor/components/components/AddLayerPanel/AddLayerPanel.component.jsx:      Events.emit('hideAddLayerPanel');
src/editor/components/components/AddLayerPanel/AddLayerPanel.component.jsx:      Events.emit('openpaymentmodal');
src/editor/components/components/CommonComponents.js:    Events.emit('entityidchange', entity);
src/editor/components/components/ActionBar/ActionBar.component.jsx:    Events.emit('hidecursor');
src/editor/components/components/ActionBar/ActionBar.component.jsx:    Events.emit('showcursor');
src/editor/components/modals/ScenesModal/ScenesModal.component.jsx:      Events.emit('updatescenegraph');
src/editor/components/modals/ProfileModal/ProfileModal.component.jsx:                    Events.emit('openpaymentmodal');
src/editor/lib/toolbar.js:  Events.emit('updatescenegraph');
src/editor/lib/toolbar.js:    Events.emit('updatescenegraph');
src/editor/lib/shortcuts.js:      Events.emit('openhelpmodal');
src/editor/lib/shortcuts.js:      Events.emit('transformmodechange', 'translate');
src/editor/lib/shortcuts.js:      Events.emit('transformmodechange', 'rotate');
src/editor/lib/shortcuts.js:      Events.emit('transformmodechange', 'scale');
src/editor/lib/shortcuts.js:      Events.emit('transformspacechange');
src/editor/lib/shortcuts.js:      Events.emit('togglegrid');
src/editor/lib/shortcuts.js:        Events.emit('objectfocus', selectedEntity.object3D);
src/editor/lib/shortcuts.js:      Events.emit('cameraperspectivetoggle');
src/editor/lib/shortcuts.js:      Events.emit('cameraorthographictoggle', 'left');
src/editor/lib/shortcuts.js:      Events.emit('cameraorthographictoggle', 'right');
src/editor/lib/shortcuts.js:      Events.emit('cameraorthographictoggle', 'top');
src/editor/lib/shortcuts.js:      Events.emit('cameraorthographictoggle', 'bottom');
src/editor/lib/shortcuts.js:      Events.emit('cameraorthographictoggle', 'back');
src/editor/lib/shortcuts.js:      Events.emit('cameraorthographictoggle', 'front');
src/editor/lib/shortcuts.js:      Events.emit('togglesidebar', { which: 'all' });
src/editor/lib/raycaster.js:      Events.emit('raycastermouseleave', null);
src/editor/lib/raycaster.js:    Events.emit(
src/editor/lib/raycaster.js:    Events.emit(
src/editor/lib/history.js:    Events.emit('historychanged', cmd);
src/editor/lib/history.js:      Events.emit('historychanged', cmd);
src/editor/lib/history.js:      Events.emit('historychanged', cmd);
src/editor/lib/history.js:    Events.emit('historychanged');
src/editor/lib/cameras.js:    Events.emit('cameratoggle', {
src/editor/lib/cameras.js:    Events.emit('cameratoggle', {
src/editor/lib/entity.js:      Events.emit('entityclone', clone);
src/editor/lib/commands/EntityRemoveCommand.js:    Events.emit('entityremoved', this.entity);
src/editor/lib/commands/EntityRemoveCommand.js:        Events.emit('entitycreated', this.entity);
src/editor/lib/commands/EntityCreateCommand.js:        Events.emit('entitycreated', this.entity);
src/editor/lib/commands/EntityCreateCommand.js:      Events.emit('entityremoved', this.entity);
src/editor/lib/commands/EntityRemoveComponentCommand.js:    Events.emit('componentremove', {
src/editor/lib/commands/EntityRemoveComponentCommand.js:    Events.emit('componentadd', {
src/editor/lib/commands/EntityAddComponentCommand.js:    Events.emit('componentadd', {
src/editor/lib/commands/EntityAddComponentCommand.js:    Events.emit('componentremove', {
src/editor/lib/commands/EntityUpdateCommand.js:    Events.emit('entityupdate', {
src/editor/lib/commands/EntityUpdateCommand.js:    Events.emit('entityupdate', {
src/editor/lib/assetsLoader.js:      Events.emit('assetsimagesload', this.images);

Answer

I'll provide an exhaustive list of all Events.emit calls, categorizing them as either creating a change or notifying of a change. I'll also note if any action events are not currently handled via an undoable command.

objectremove
Purpose: Notify a change
Emitted in: src/editor/index.js
helperremove
Purpose: Notify a change
Emitted in: src/editor/index.js
entityselect
Purpose: Create a change
Emitted in: src/editor/index.js, src/editor/components/scenegraph/SceneGraph.js
Note: This is not currently handled via an undoable command.
objectselect
Purpose: Notify a change
Emitted in: src/editor/index.js
inspectortoggle
Purpose: Create a change
Emitted in: src/editor/index.js
Note: This is not currently handled via an undoable command.
objectfocus
Purpose: Create a change
Emitted in: src/editor/index.js, src/editor/components/scenegraph/Entity.js, src/editor/lib/shortcuts.js
Note: This is not currently handled via an undoable command.
updatescenegraph
Purpose: Notify a change
Emitted in: src/editor/components/scenegraph/Toolbar.js, src/editor/lib/toolbar.js, src/editor/components/modals/ScenesModal/ScenesModal.component.jsx
opensigninmodal, openpaymentmodal, openscenesmodal, openscreenshotmodal, openprofilemodal, opengeomodal, openhelpmodal
Purpose: Create a change
Emitted in: Various files (e.g., src/editor/components/scenegraph/Toolbar.js, src/editor/components/components/ProfileButton/ProfileButton.component.jsx, src/editor/lib/shortcuts.js)
Note: These are not currently handled via undoable commands.
sceneplayingtoggle
Purpose: Create a change
Emitted in: src/editor/components/scenegraph/Toolbar.js
Note: This is not currently handled via an undoable command.
togglesidebar
Purpose: Create a change
Emitted in: src/editor/components/Main.js, src/editor/lib/shortcuts.js
Note: This is not currently handled via an undoable command.
entityupdate
Purpose: Notify a change
Emitted in: src/editor/components/components/Mixins.js, src/editor/lib/commands/EntityUpdateCommand.js
entityidchange
Purpose: Notify a change
Emitted in: src/editor/components/components/CommonComponents.js
hidecursor, showcursor
Purpose: Create a change
Emitted in: src/editor/components/components/ActionBar/ActionBar.component.jsx
Note: These are not currently handled via undoable commands.
hideAddLayerPanel
Purpose: Create a change
Emitted in: src/editor/components/components/AddLayerPanel/AddLayerPanel.component.jsx
Note: This is not currently handled via an undoable command.
transformmodechange, transformspacechange
Purpose: Create a change
Emitted in: src/editor/components/viewport/TransformToolbar.js, src/editor/lib/shortcuts.js
Note: These are not currently handled via undoable commands.
opentexturesmodal
Purpose: Create a change
Emitted in: src/editor/components/widgets/TextureWidget.js
Note: This is not currently handled via an undoable command.
togglegrid
Purpose: Create a change
Emitted in: src/editor/lib/shortcuts.js
Note: These are not currently handled via undoable commands.
cameraperspectivetoggle, cameraorthographictoggle
Purpose: Create a change
Emitted in: src/editor/lib/shortcuts.js
Note: These are not currently handled via undoable commands.
raycastermouseleave
Purpose: Notify a change
Emitted in: src/editor/lib/raycaster.js
historychanged
Purpose: Notify a change
Emitted in: src/editor/lib/history.js
cameratoggle
Purpose: Create a change
Emitted in: src/editor/lib/cameras.js
Note: This is not currently handled via an undoable command.
entityclone
Purpose: Notify a change
Emitted in: src/editor/lib/entity.js
entityremoved
Purpose: Notify a change
Emitted in: src/editor/lib/commands/EntityRemoveCommand.js
entitycreated
Purpose: Notify a change
Emitted in: src/editor/lib/commands/EntityRemoveCommand.js, src/editor/lib/commands/EntityCreateCommand.js
componentremove, componentadd
Purpose: Notify a change
Emitted in: src/editor/lib/commands/EntityRemoveComponentCommand.js, src/editor/lib/commands/EntityAddComponentCommand.js
assetsimagesload
Purpose: Notify a change
Emitted in: src/editor/lib/assetsLoader.js

This list covers all the Events.emit calls in the codebase. Many of the events that create changes (like entityselect, inspectortoggle, and various modal openings) are not currently handled via undoable commands. Implementing undoable commands for these actions could improve the editor's undo/redo functionality and overall user experience.

…feature

vincentfretin · 2024-08-27T11:56:10Z

Probably entityidchange (but not used on 3dstreet) and entityclone (we would need to serialize to json the cloned entity) may be good candidates for undo. The other events are mainly related to the UI.

The purpose may not be accurate, it depends of the point of view, it can be considered an action that changes the React ui or select box in viewport (entityselect, objectselect).
sceneplayingtoggle purpose is both notify and an action, it triggers the change historyDisabled in history.js
inspectortoggle is actually between an action and notify, one change is done (this.opened set to true/false) before the event is emitted to do other changes.

…moveComponentCommand to ComponentRemoveCommand

…mand similar to EntityUpdateCommand

… updateEntity from entityupdate command to lib/entity.js

…etached

kfarr · 2024-08-27T22:38:37Z

Wow this is a lot of refactoring! I took some time to look at all the changes

Here are some things I'm noticing

instead of emiting event, AFRAME.INSPECTOR.execute('componentadd', { ...
instead of bespoke create entity functions, using AFRAME.INSPECTOR.execute('entitycreate', newEntityObject);
default to create new entity as child of street-container, or if the object has parentEl property defined use that eg. newEntityObject.parentEl = customGroupEl;

things to test:

- behavior of adding entity -- will it go into segment container? or streets-container?
- all custom layers, street layers, random assets from each group
- test deleting those layers and others, note the message change should include entity name. also try undoing the remove
- go through the "flow": open old file > make changes > save new file with these changes > reload app > load the file to see if it displays correctly
- save / open new files

bugs found;

undoing changing showground property on street component does not work (but it is the component that is nonstandard as it uses an additional synchronize property, therefore nothing to fix in this pr)

other comments:

the claude + cursor thing is awesome
of that list it generated of things that could be turned into a command, I think selecting/unselecting entities is one of the highest priorities. It can be useful for both ai or multi-user as a selection event is important as a form of communication. Selection / deselection of layers in the undo stack is default behavior in figma, it is worth noting that some users don't seem to like it: https://forum.figma.com/t/user-preference-to-include-or-exclude-object-selection-in-undo/5333/22

vincentfretin · 2024-08-29T10:40:10Z

I implemented the undo for the duplicate action (d shortcut).
I'm happy with the implementation, please do some more tests, if all is okay, we can merge.

vincentfretin · 2024-08-29T12:06:33Z

For the next things, I created:

vincentfretin · 2024-08-29T12:10:06Z

undoing changing showground property on street component does not work (but it is the component that is nonstandard as it uses an additional synchronize property, therefore nothing to fix in this pr)

No that was a bug in my implementation, fixed by 0ca98b3

… be sure we include only properties we set explicitely if we undo and then redo

vincentfretin · 2024-08-29T15:54:12Z

The latest commit is an edge case I have in my project, that may not be that important in your case. But I still verified we only get in the undo stack only the object with properties we set, and not all the properties.
Here when setting the street-geo component, updating it by changing the geoloc again and then manually removing the component:

…bject is recycled

vincentfretin · 2024-08-30T11:23:54Z

Some notes:
Having an id on an entity is mandatory for undo to work, especially in the case of the following steps: update position, delete, undo delete, undo update position, the recreated entity when we undo the delete will be different, that's why we keep entityId in the command entityupdate, and not the entity like it was the case when entityupdate was the only command we implemented. So each command will verify if we have an id, if we don't, we generate one with nanoid.
For entityremove command, we keep on the command an unloaded DOM clone (prepareForSerialization), so the deleted entity threejs resources are freed. Later if we need to serialize the undo stack, we can serialize that with elementToObject that is defined in #537 and the createEntity function will need to support creating the children.

vincentfretin · 2024-08-30T11:32:27Z

I also tested the implementation in my project. I didn't find any other issue.
There is a command that I'll need in my project is transaction/batch command to run and undo atomically a set of commands, I didn't implement it yet.

kfarr · 2024-08-30T16:12:48Z

@vincentfretin thanks for all this work. Regarding nanoid, I think it's great to have autoid on our entities. I think the only issue with this is sometimes a user will be surprised to see a random ID or wish to change it. Therefore in a future ticket we could allow the end-user to more easily change data-layer-display-name so they can quickly change it to something else for display purposes while retaining the id underneath.

vincentfretin · 2024-08-30T16:50:47Z

The user shouldn't see the random id anywhere, if you still see it somewhere let me know it's an issue.
I modified the getEntityDisplayName function to never show the id, priority is displaying data-layer-name, class, mixin currently.
I now also use getEntityDisplayName for the delete dialog.

kfarr · 2024-08-30T22:17:56Z

did final testing, all looks good to me

feedback

it looks like all drag-n-drop objects are placed in User Layer root by default and do not get placed as children of segments -- that is ok with me just confirming this is desired behavior
note: intersection drag-n-drop does not respect 'position' from drop event, that is a separate task drag-and-drop to respect position for intersection, svg, gltf #810
layer visibility via checkbox in scene graph is not an undo-able action, however editing visible as a property of material IS undo-able. We may want to revisit this in the future as part of save layer visibility state, layer visibility should be undo-able #793

vincentfretin · 2024-08-31T05:11:30Z

it looks like all drag-n-drop objects are placed in User Layer root by default and do not get placed as children of segments -- that is ok with me just confirming this is desired behavior

Yes that's correct, you said you wanted something simple for the drag and drop. ;)

vincentfretin added 12 commits August 26, 2024 11:04

add EntityAddComponentCommand and EntityRemoveComponentCommand

7801121

add EntityCreateCommand and modify createLayerFunctions to use it

d9cf7f1

use correct componentadd or entityupdate commands when changing stree…

6835588

…t-geo

update createStreetmixStreet to use command

6127e84

remove unused entitycreate, replace it by updatescenegraph, remove un…

85eda94

…used addEntity

modify createEntity and createEntityOnPosition to use EntityCreateCom…

cfcffc0

…mand

emit entityremoved event when removing entity to update scene graph

a289282

add EntityRemoveCommand

cff2b7c

add new createUniqueId function

5dd1ae8

don't show the random entity id in scene graph, use getEntityDisplayN…

15e361d

…ame in delete confirm dialog

get again the entity from id for undo to properly work after undoing …

50348c9

…remove entity

always set an id for created entity

0384ac9

vincentfretin requested a review from kfarr August 26, 2024 13:34

vincentfretin self-assigned this Aug 26, 2024

Remove unused m shortcut that was used in the removed motion capture …

f39339f

…feature

vincentfretin and others added 12 commits August 27, 2024 16:13

rename EntityAddComponentCommand to ComponentAddCommand, and EntityRe…

c6786b1

…moveComponentCommand to ComponentRemoveCommand

use a single payload param for ComponentAddCommand/ComponentRemoveCom…

20ae117

…mand similar to EntityUpdateCommand

free reference to removed entity

5667cc7

simplify command execution

9a1ff8b

return in execute so we get the created entity for entitycreate command

24f2483

recreate createEntity and use it in the entitycreate command

4927b8b

simplify updateEntity function used in PropertyRow component and move…

46724a0

… updateEntity from entityupdate command to lib/entity.js

move emit entityupdate in updateEntity

4172a79

Remove redundant removeObject call, this is already called on child-d…

e3bb79e

…etached

Remove unneeded iife in addHelper

ce177fe

Use const to be consistent in this file

6e654f9

Merge branch 'main' into undo-plus

9b445b5

vincentfretin mentioned this pull request Aug 29, 2024

Undo/redo selecting/unselecting entities and add an option to disable that behavior #802

Open

use getDOMAttribute instead of getAttribute to store the old value to…

9612ac5

… be sure we include only properties we set explicitely if we undo and then redo

vincentfretin added 5 commits August 29, 2024 18:06

make a copy of the object returned by getDOMAttribute otherwise the o…

629fb2d

…bject is recycled

really remove exposed off/on/emit on inspector

6bf2daa

add config file to set defaultParent config

1519c50

move debugUndoRedo to config

1f07920

simplify condition

3e512c4

vincentfretin force-pushed the undo-plus branch from 75492d0 to 3e512c4 Compare August 30, 2024 07:49

avoid importing from .component.jsx files

39e7a28

vincentfretin mentioned this pull request Aug 30, 2024

Avoid importing from .component.jsx files #796

Closed

vincentfretin added 2 commits August 30, 2024 09:52

simplify condition

999d566

remove comment, we put back createEntity with doc in entity.js

10486aa

vincentfretin added 2 commits August 30, 2024 14:51

add empty line

0abebfc

add comment

38b5175

vincentfretin added 2 commits August 30, 2024 18:59

remove dead code

798c226

remove console.log

e38cfd4

Merge branch 'main' into undo-plus

2a044d8

kfarr merged commit 448d572 into main Aug 30, 2024
1 check passed

kfarr deleted the undo-plus branch August 30, 2024 22:19

kfarr mentioned this pull request Aug 30, 2024

child layers in newly placed street layer from add entity panel not showing in scene graph #792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Undo create/remove entity, add/remove component #789

Undo create/remove entity, add/remove component #789

vincentfretin commented Aug 26, 2024

vincentfretin commented Aug 27, 2024 •

edited

Loading

vincentfretin commented Aug 27, 2024

kfarr commented Aug 27, 2024 •

edited

Loading

vincentfretin commented Aug 29, 2024

vincentfretin commented Aug 29, 2024

vincentfretin commented Aug 29, 2024

vincentfretin commented Aug 29, 2024

vincentfretin commented Aug 30, 2024 •

edited

Loading

vincentfretin commented Aug 30, 2024

kfarr commented Aug 30, 2024 •

edited

Loading

vincentfretin commented Aug 30, 2024

kfarr commented Aug 30, 2024 •

edited by vincentfretin

Loading

vincentfretin commented Aug 31, 2024 •

edited

Loading

Undo create/remove entity, add/remove component #789

Undo create/remove entity, add/remove component #789

Conversation

vincentfretin commented Aug 26, 2024

vincentfretin commented Aug 27, 2024 • edited Loading

Prompt

Answer

vincentfretin commented Aug 27, 2024

kfarr commented Aug 27, 2024 • edited Loading

vincentfretin commented Aug 29, 2024

vincentfretin commented Aug 29, 2024

vincentfretin commented Aug 29, 2024

vincentfretin commented Aug 29, 2024

vincentfretin commented Aug 30, 2024 • edited Loading

vincentfretin commented Aug 30, 2024

kfarr commented Aug 30, 2024 • edited Loading

vincentfretin commented Aug 30, 2024

kfarr commented Aug 30, 2024 • edited by vincentfretin Loading

vincentfretin commented Aug 31, 2024 • edited Loading

vincentfretin commented Aug 27, 2024 •

edited

Loading

kfarr commented Aug 27, 2024 •

edited

Loading

vincentfretin commented Aug 30, 2024 •

edited

Loading

kfarr commented Aug 30, 2024 •

edited

Loading

kfarr commented Aug 30, 2024 •

edited by vincentfretin

Loading

vincentfretin commented Aug 31, 2024 •

edited

Loading