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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editor: Fix persistent history. #28405

Merged
merged 3 commits into from
May 17, 2024
Merged

Editor: Fix persistent history. #28405

merged 3 commits into from
May 17, 2024

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented May 17, 2024

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 Mugen87 added this to the r165 milestone May 17, 2024

} );

signals.scriptRemoved.add( function () {

signals.objectChanged.dispatch( editor.selected );
if ( editor.selected !== null ) signals.objectChanged.dispatch( editor.selected );
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Mugen87 Mugen87 merged commit b392786 into mrdoob:dev May 17, 2024
11 checks passed
@Mugen87 Mugen87 changed the title Editor: Fix serialization of commands. Editor: Fix persistent history. May 17, 2024
@ycw
Copy link
Contributor

ycw commented May 17, 2024

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

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

Thanks.

@ycw
Copy link
Contributor

ycw commented May 17, 2024

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

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 17, 2024

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
Copy link
Collaborator Author

Mugen87 commented May 17, 2024

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
Copy link
Contributor

ycw commented May 18, 2024

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

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

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 :)

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.

Editor: Runtime error in history.
2 participants