Editor: Creates stub command during restore

[ycw]

Related: #28345 ( issue 1 )

This PR solved issue 1 in #28345, by creating a stub command when commands get restored from json, in this way we bypassed the constructor. This approach is enlightened by #28360 (comment) :D

can close #28360

[Mugen87]

I think it's better to actually update the commands. I appreciate your effort but this PR feels like we work around the actual issue (which eventually remains unsolved).

E.g. RemoveObjectCommand should look something like this:

import { Command } from '../Command.js';

import { ObjectLoader } from 'three';

/**
 * @param editor Editor
 * @param object THREE.Object3D
 * @constructor
 */
class RemoveObjectCommand extends Command {

	constructor( editor, object = null ) {

		super( editor );

		this.type = 'RemoveObjectCommand';

		if ( object !== null ) {

			this.name = editor.strings.getKey( 'command/RemoveObject' ) + ': ' + object.name;

			this.object = object;
			this.parent = ( object !== undefined ) ? object.parent : undefined;
			if ( this.parent !== undefined ) {

				this.index = this.parent.children.indexOf( this.object );

			}


		}

	}

	execute() {

		this.editor.removeObject( this.object );
		this.editor.deselect();

	}

	undo() {

		this.editor.addObject( this.object, this.parent, this.index );
		this.editor.select( this.object );

	}

	toJSON() {

		const output = super.toJSON( this );

		output.object = this.object.toJSON();
		output.index = this.index;
		output.parentUuid = this.parent.uuid;

		return output;

	}

	fromJSON( json ) {

		super.fromJSON( json );

		this.parent = this.editor.objectByUuid( json.parentUuid );

		if ( this.parent === undefined ) {

			this.parent = this.editor.scene;

		}

		this.index = json.index;

		this.object = this.editor.objectByUuid( json.object.object.uuid );

		if ( this.object === undefined ) {

			const loader = new ObjectLoader();
			this.object = loader.parse( json.object );

		}

	}

}

export { RemoveObjectCommand };

Notice how object is now optional and fromJSON() also restores the name property.

[ycw]

I think it's better to actually update the commands. I appreciate your effort but this PR feels like we work around the actual issue (which eventually remains unsolved).

Thanks,

E.g. RemoveObjectCommand should look something like this:

Your approach will make the constructor hard to reason about, for example SetMaterialMapCommand ...

three.js/editor/js/commands/SetMaterialMapCommand.js

Line 13 in de367cb

constructor( editor, object, mapName, newMap, materialSlot ) {

... which has several parameters needed to be validated,

so it will get rejected by @mrdoob for the same reason of using arguments.length>1

[ycw]

It is better to design command constructor that all mandatory parameters are indeed required, instead of making them optional just for satisfying the case of 'restoring', or else anyone trying to learn the current system would be confused, note that commands may have real optional parameters, e.g. SetPositionCommand. Rather, we should do the opposite, let the consumer, History.fromJSON in the case, finds a way to satisfy the constructor, or just not using the constructor if mandatory constructor parameters are missing(i.e. they don't get serialized) ... That's why this PR tries bypass the constructor, in this way, command constructor only focuses on "constructing a command object", not bothered by handling recovery, so that #28360 (comment) is satisfied.

[Mugen87]

I'm afraid this is against the way we implement fromJSON() in the core library. A good example is the curve classes and how they implement their ctors and fromJSON(). I'll file a PR with updated command classes in the next days to demonstrate how the code will look like.

[ycw]

A good example is the curve classes and how they implement their ctors and fromJSON()

If the 'curve classes' is referring to:

three.js/src/extras/core/CurvePath.js

Line 244 in de367cb

this.curves.push( new Curves[ curve.type ]().fromJSON( curve ) );

then I can tell why 'curve classes' works and why it's not suitable for our case.... If we look at ctors of all curves carefully, we'll find that the only operator in used is assign =, meaning that it won't cause any runtime issues even if a mandatory parameter is missing, the state will just be assigned undefined, so its safe to do so before calling fromJSON() to restore the currently-undefined states.

However, in commands, as simple as SetColorCommand, there's not only assigning value, but also accessing property, calling function etc... which we've to guard them or else it will throw when constructing at runtime:

three.js/editor/js/commands/SetColorCommand.js

Line 22 in de367cb

this.oldValue = ( object !== undefined ) ? this.object[ this.attributeName ].getHex() : undefined;

i.e. If we're using command ctor to construct a placeholder command, then its inevitable that we need if/else to guard those operations, my initial attempt was using if (arguments.length > 1) { } (single checking), and it was rejected; then my 2nd attempt was using an sentinel object at first parameter (also, single checking), but was also rejected, so I suspect that if you're making mandatory parameters to be optional and guards them with multiple checkings in the ctor, it'll also be rejected, for the same reason: Confusion, this time the confusion comes from "why mandatory params are optional in the first place"

---

'll file a PR with updated command classes in the next days to demonstrate how the code will look like.

Good luck :D