Editor: Defers Commands initialization

[ycw]

Update: This post has converted to draft as #28398 seems to be more appropriated, see #28398 (comment) for reason why constructor should not be bothered by states recovery, thanks.

---

Related: #28345 ( issue 1 )

Issue: Commands don't respect current recovery mechanics:

• In History.js, fromJSON():

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 performing 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.

Solution (This PR): Defers command initialization

In History.js, .fromJSON() is now constructing command with an Defer object, as a sentinel:

const cmd = new Commands[ cmd.type ]( new Defer( this.editor ) )

Then in each command constructor, guarding initialization via check against the sentinel:

// AddObjectCommand.js
constructor( editor , object ) {
   if ( editor instanceof Defer ) { // guard
     super( editor.unwrap() );
     return;
   } 
   // init states here

This way is more explicit than using arguments.length>1 as a guard, solved #28345 (comment)

[mrdoob]

Same as with other PRs... Isn't there a simpler solution for this?

[Mugen87]

Because of #28395, I had a closer look at the editor's command implementation I don't think this PR goes in the right direction. The root cause is the way the history restores commands in History.fromJSON() which is not correct, imo.

History.fromJSON() restores commands like so:

const cmd = new Commands[ cmdJSON.type ]( this.editor ); // creates a new object of type "json.type"

This is obviously wrong since many mandatory parameters are missing depending on the command type.

The idea is to make all parameters optional except for the editor reference. In context of RemoveObjectCommand, you can apply the object while constructing the command, but it is also valid to not apply it.

This does currently not work since RemoveObjectCommand requires object so when not provided a runtime errors occurs (see #28395).

Considering all this, the ideal fix to me is:

a) make sure all parameters except for editor are optional in all commands classes.
b) make sure fromJSON() turns a command into the same state like when using the constructor with all parameters.

[ycw]

The root cause is the way the history restores commands in History.fromJSON() which is not correct, imo.

👍 correct; New approach delivered #28398 :D