Allow storing preferences in mirabuf files and refactor preferences system [SYNTH-194] and [SYNTH-190] - #1371
Conversation
6e3720b to
5bee5e4
Compare
|
I'm working on getting the checks to pass but this PR is ready for review |
|
Chorus detected one or more security issues with this pull request. See the Checks tab for more details. As a reminder, please follow the secure code review process as part of the Secure Coding Trust Commitment requirement. |
ebf382b to
ab4a4bf
Compare
There was a problem hiding this comment.
@azaleacolburn do you have any ideas for a long term solution to the autodesk registry getting added to the lockfile?
azaleacolburn
left a comment
There was a problem hiding this comment.
I have a few suggestions, feel free to ignore :3
| if (this.miraType === MiraType.FIELD && !PreferencesSystem.hasFieldPreferences(this.assemblyId)) { | ||
| this._fieldPreferences = defaultFieldPreferences() | ||
| editor.migrateDevtoolFieldData(this._fieldPreferences) | ||
| this._fieldPreferences = { | ||
| ...this._fieldPreferences, | ||
| ...editor.getUserData("synthesis:field_preferences"), | ||
| } | ||
| } else if (this.miraType === MiraType.ROBOT && !PreferencesSystem.hasRobotPreferences(this.assemblyId)) { | ||
| this._robotPreferences = defaultRobotPreferences() | ||
| editor.migrateDevtoolRobotData(this._robotPreferences) | ||
| this._robotPreferences = { | ||
| ...this._robotPreferences, | ||
| ...editor.getUserData("synthesis:robot_preferences"), | ||
| } | ||
| } |
There was a problem hiding this comment.
This is pedantic, but:
- I think using the
satisfiesoperator on every object literal would be a good practice. - This section is hard for me to read for some reason and I think some whitespace would be helpful, especially before the preferences objects get created.
There was a problem hiding this comment.
Also, I don't like the amount of repeated structure within this function and within resetPreferences().
I could imagine a really cool solution where some function matches a set of functions, properties, and strings against an enum and then always runs the same structure of statements just with those variable arguments and functions. However, this would probably be unreadable.
If you have any (better) ideas about how to reduce repeated structure here, I would appreciate it.
There was a problem hiding this comment.
I agree it could be better but the typing is really obnoxious and I would prefer to make that refactor a separate ticket if you think it's important
There was a problem hiding this comment.
If I add a scoring/protected zone, then click reset (in the configure panel), it doesn't immediately remove that added zone until I either respawn the asset again or enter into the configuration menu.
The toast popup should either tell users they need to respawn it for changes to take effect, or it should immediately update.
Task
SYNTH-194
SYNTH-190
SYNTH-118
Symptom & Solution
For 194:
For 190:
For 118:
Verification
bun run assetpackto get the new modelsBefore merging, ensure the following criteria are met: