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

don't save empty records in asset editor #1681

Merged
merged 11 commits into from
Nov 19, 2022
Merged

don't save empty records in asset editor #1681

merged 11 commits into from
Nov 19, 2022

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Oct 16, 2022

Its redundant to add TM:PE data to an asset when there are no TM:PE rules. This redundancy has some downsides:

  • If there is a bug in TMPE placing an asset would cause error even if asset creator created no TM:PE rules.
  • this could potentially cause forward/backward compatibility issues
  • performance (speed/memory)

All this inconvenience can be avoided by not recording Traffic rules if there is none.

Changes:

  • Added bool ?Get/SetHasTrafficLights() which returns null/resets if traffic light value is not forced.
  • added IsDefault() to IRecordables
  • asset data is not saved (returns null) if all records have default traffic rules.
  • null guards

TMPE.zip

@kianzarrin kianzarrin added technical Tasks that need to be performed in order to improve quality and maintainability MASS EDIT The mass edit tool Asset Editor Issue related to TM:PE support in content editors labels Oct 16, 2022
@kianzarrin kianzarrin self-assigned this Oct 16, 2022
Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

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

Nice
How big is empty data record?

Base automatically changed from clear-node to master October 28, 2022 15:58
@kianzarrin kianzarrin marked this pull request as ready for review November 18, 2022 15:19
Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

Looks ok, question is: Is it backward compatible with existing Move It exports, e.g. those from Allbuilds.org? I mean bool to bool? change 🤔

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Nov 19, 2022

@krzychu124 good point. I saved traffic light move it record with old TMPE and loaded with new TMPE and it worked! both off and on state.
its nice to know that binary serialization is cooperating for once!

@kianzarrin kianzarrin merged commit 8b6fd00 into master Nov 19, 2022
@kianzarrin kianzarrin deleted the default-record2 branch November 19, 2022 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Asset Editor Issue related to TM:PE support in content editors MASS EDIT The mass edit tool technical Tasks that need to be performed in order to improve quality and maintainability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants