Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editor: Fixed recovery system #28345

Closed
wants to merge 2 commits into from
Closed

Conversation

ycw
Copy link
Contributor

@ycw ycw commented May 12, 2024

Updates: This post has converted to draft, the below issues will be fixed in standalone PRs


The issues and solutions:

Issue 1: Commands don't respect current recovery mechanics:

const cmd = new Commands[ cmdJSON.type ]( this.editor ); // (A)

A placeholder command is created in (A), however, some commands don't handle this case, i.e. commands may throw at (A), or some states may wrongly be assigned to undefined if they depends on constructor parameters but now missing, in this case the error will be thrown when Undo/Redo instead.

To reproduce:

  1. Go to https://threejs.org/editor/
  2. In SETTINGS tab, enable persistent in History section
  3. Add a Box
  4. Select Box in outliner, then in MATERIAL tab, update Color to red
  5. Refresh the page
  6. Open console first, then press editor's Edit>Undo
  7. You should see an error about Cannot read properties of undefined (reading 'color') in console.

This PR fixed that by unifying all commands to have a guard in constructor in order to respect the recovery mechanics:

if (arguments.length > 1) { // guard
  // init states here

Issue 2: SetGeometryCommand failed to UNDO/REDO

Because it wrongly serializes oldGeometry as current object's geometry.

To reproduce:

  1. Go to https://threejs.org/editor/
  2. In SETTINGS tab, enable persistent in History section
  3. Add a Box
  4. Select Box in outliner, then in GEOMETRY tab, update Width from 1 to 2
  5. Refresh the page
  6. Press Edit>Undo
  7. You should see the box's width is of 2, not 1

This PR fixed that by serializing it as this.oldGeometry.toJSON()

Isseu 3: All material related commands SetMaterialXXXCommand are failed to UNDO/REDO

In execute() and undo(), the material is referencing to the one cached at construction time, it may not be the same material that materialChanged signal is dispatching for.

This PR fixed that by getting material via given object, materialSlot in execute() and undo(), instead of using cached one, s.t. it must be the same material as the one materialChanged is dispatching for:

const material = this.editor.getObjectMaterial( this.object, this.materialSlot )

// do stuff with material

this.editor.signals.materialChanged.dispatch( this.object, this.materialSlot )

Issue 4: History entries don't honor newly changed locale

When enabled persistent history, then make some changes, then changes SETTINGS>Language, then refresh the page; The history entries will be shown in previous language, not the current language.

The cause is that command name is restored from json, not re-computed during recovery, so it won't respect current editor's language settings.

This PR doesn't fixed this issue. I'll submit another PR after this PR get merged


How to test

  1. Make sure that 'persistent' is checked in SETTINGS>HISTORY section

  2. Create new project (File>New Project>Empty)

  3. Make changes, see below (editor will create commands for history)

  4. Refresh the page (make sure states have been stored to IndexedDB before doing so, i.e. wait at least 0.5s :D)

  5. Press Edit>Undo n times until no available Undos
    (in order to test commands' undo())

  6. Press Edit>Redo n times until no available Redos
    (in order to test commands' execute())


Make changes to test AddObjectCommand:

  1. Add>Mesh>Box

Make changes to test AddScriptCommand:

  1. Select default scene in outliner
  2. In SCRIPT tab, press 'NEW' to add a script

Make changes to test MoveObjectCommand:

  1. Add>Mesh>Box
  2. Add>Mesh>Ring
  3. Reorder Ring to come before Box in outliner

Make changes to test MultiCmdsCommand:

(n/a, this command currently has no consumers; should work :D)

Make changes to test RemoveObjectCommand:

  1. Add>Mesh>Box
  2. Select the Box in outliner, press 'delete' key

Make changes to test RemoveScriptCommand:

  1. Select default scene in outliner
  2. In SCRIPT tab, press NEW to add a script, then press REMOVE to remove it

Make changes to test SetColorCommand:

  1. Add>Light>Directional
  2. In OBJECT tab, change Color to red

Make changes to test SetGeometryCommand:

  1. Add>Mesh>Tube
  2. In GEOMETRY tab, in 'Path' row, press '+' to add a point
  3. Then, modify the newly added point's coords
  4. (editor will create two SetGeometryCommands, adding+modifying)

Make changes to test GeometryValueCommand:

  1. Add>Mesh>Box
  2. In GEOMETRY tab, changes Name

Make changes to test SetMaterialColorCommand:

  1. Add>Mesh>Box
  2. In MATERIAL tab, changes Color to red

Make changes to test SetMaterialCommand:

  1. Add>Mesh>Box
  2. In MATERIAL tab, select MeshBasicMaterial for Type

Make changes to test SetMaterialRangeCommand:

  1. Add>Mesh>Box
  2. In MATERIAL tab, selet MeshPhysicalMaterial for Type
  3. Changes 'Thin-Film Thickness Map' min/max

Make changes to test SetMaterialValueCommand:

  1. Add>Mesh>Box
  2. In MATERIAL tab, check Wireframe

Make changes to test SetMaterialVectorCommand:

  1. Add>Mesh>Box
  2. In MATERIAL tab, input an image for Normal Map
  3. Enable Normal Map by checking the check box next to image input
  4. Then change its scale x to 2

Make changes to test SetPositionCommand:

  1. Add>Mesh>Box
  2. In OBJECT tab, change Position x to 2

Make changes to test SetRotationCommand:

  1. Add>Mesh>Box
  2. In OBJECT tab, change Rotation x to 45deg

Make changes to test SetScaleCommand:

  1. Add>Mesh>Box
  2. In OBJECT tab, change Scale x 2

Make changes to test SetSceneCommand:

(tbd; should work :D)

Make changes to test SetScriptValueCommand:

  1. Select default Scene in outliner
  2. In SCRIPT tag, press NEW to add a script
  3. Enter name for the script

Make changes to test SetUuidCommand:

  1. Select default Scene in outliner
  2. In OBJECT tab, in UUID row, press NEW

Make changes to test SetValueCommand:

  1. Add>Mesh>Box
  2. In OBJECT tab, check 'Visible'

Thank You for Your Patience

馃憦

@mrdoob
Copy link
Owner

mrdoob commented May 13, 2024

I'm not sure about the solution for issue 1...

if (arguments.length > 1) { // guard
 // init states here

This code would be confusing for anyone trying to learn the current system.

Are there other solutions?

@mrdoob
Copy link
Owner

mrdoob commented May 13, 2024

Also, would it possible to split this in multiple PRs?

@ycw
Copy link
Contributor Author

ycw commented May 13, 2024

Are there other solutions?

For issue 1, new approach is submitted, see #28360

Also, would it possible to split this in multiple PRs?

For issue 2, see #28361
For issue 3, see #28362
For issue 4, n/a

@ycw
Copy link
Contributor Author

ycw commented May 16, 2024

I made a simple fix for issue 1, please see #28398

@Mugen87
Copy link
Collaborator

Mugen87 commented May 18, 2024

This PR should be obsolete now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants