Editor: Fix persistent history.

[Mugen87]

Fixed #28395.

Closes #28360. Closes #28398. Closes #28362.

Description

This PR ensures commands can now properly serialized/deserialized when persistent history is enabled.

It makes sure all commands have proper default values and don't throw exceptions in their ctors when default parameters are used.

The PR also makes sure material commands use the correct material reference. This is done by not caching the material in the command but the material slot instead. With this information, the correct material can be requested with the object. Meaning this pattern:

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

[Mugen87]

The assumptions made in #28270 are not right. When scripts are added or removed, an object is not necessarily selected. So when undo/redo script commands, editor.selected can be null. In this case, a runtime error occurs in Viewport.Controls.js.

[ycw]

Would you mind explain how this approach is less confusing than #28345 (comment) ?

and how it is simpler than #28360 (comment) ?

Thanks.

[ycw]

I'm little lost, because other related PRs just all get closed in a sudden without a reason 🤔

[Mugen87]

They were closed because the changes should not be necessary anymore.

When you look at the constructor code of the commands, it already handled undefined arguments. This PR just introduced real default values instead of checking the arguments parameter. It also made sure no runtime errors occur when the commands are created with just the edior assigned.

So I think this solution better fits to what already has been implemented.

[Mugen87]

Even if your PRs were not merged, they were very helpful to understand the issues in the editor and developing this solution. When writing the release notes, I'll definitely add your as a contributor for this PR.

[ycw]

constructor code of the commands, it already handled undefined arguments.

If I understand correctly, this solution cannot reflect the fact that constructor parameters other than editor will be either all presented or all omitted, so there's few confusions still remain here ...

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

Line 22 in cb24e42

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

By reading the constructor signature, this.object[this.attributeName] can be undefined if attributeName is not present, in turn .getHex() could throw at runtime, "How to explain that a if-guard is needn't for this.object[this.attributeName]?"

... and there ...

three.js/editor/js/commands/SetPositionCommand.js

Line 13 in cb24e42

constructor( editor, object = null, newPosition = null, optionalOldPosition = null ) {

"How to explain that non optional parameters have default values?"

---

Even if your PRs were not merged, they were very helpful to understand the issues in the editor and developing this solution.

I appreciate that you submit a solution to show your though, but this solution is not being less confusing than the other solutions;

Since this solution had been adopted and others had been rejected, so could we reach a consensus on that 'confusion' doesn't matter now? [Yes/No] The reply would help others to find a better solution in the future. Thanks :)