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

--Attributes/Managers Maintenance : Separate Monolithic ObjectAttributes; Remove unnecessary abstract method; Build JSON From String #2177

Merged
merged 7 commits into from
Aug 8, 2023

Conversation

jturner65
Copy link
Contributor

@jturner65 jturner65 commented Aug 7, 2023

Motivation and Context

This PR takes care of 4 longstanding issues with the Attributes/Managers subsystem :

  • It separates the AbstractObject, Object and Stage attributes into separate source files. These were originally combined when the attributes/managers subsystem was in a more flat layout. Breaking them apart so they are easier to manage and understand and match their respective managers' structure.
  • It removes an unnecessary abstract function path that was unused by most managers. Only Stage Attributes and Object Attributes should ever need to query whether an asset handle is a valid Primitive asset handle, so this removes the query from all managers other than those.
  • It adds a function to ManagedFileBasedContainer to build a Managed object/Attributes directly from an appropriately configured string without needed to load a file from disk.
  • Renames a file to match the class template it describes. -sigh-

How Has This Been Tested

All C /Python tests pass

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@jturner65 jturner65 requested a review from aclegg3 August 7, 2023 16:34
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Aug 7, 2023
@jturner65 jturner65 requested a review from 0mdc August 7, 2023 16:39
No need for every manager to attempt to query whether a handle is a primitive asset or not, only stages and rigid objects consume these.
@jturner65 jturner65 changed the title --Break Abstract Object Attributes/Object Attributes/Stage Attributes into separate files. --Attributes/Managers Maintenance : Separate Monolithic ObjectAttributes; Remove unnecessary AttrManager abstract method Aug 7, 2023
@jturner65 jturner65 force-pushed the Attr_RefactorObjAttr branch from e303793 to ad6ebe1 Compare August 7, 2023 17:53
--'abstract' and 'base' are redundant here, so renamed to match name of class template.
@jturner65 jturner65 force-pushed the Attr_RefactorObjAttr branch from ad6ebe1 to 70a1360 Compare August 7, 2023 19:55
@jturner65 jturner65 changed the title --Attributes/Managers Maintenance : Separate Monolithic ObjectAttributes; Remove unnecessary AttrManager abstract method --Attributes/Managers Maintenance : Separate Monolithic ObjectAttributes; Remove unnecessary abstract method; Build JSON From String Aug 7, 2023
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

LGTM

src/esp/core/managedContainers/ManagedFileBasedContainer.h Outdated Show resolved Hide resolved
@jturner65 jturner65 merged commit d2450b3 into main Aug 8, 2023
@jturner65 jturner65 deleted the Attr_RefactorObjAttr branch August 8, 2023 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants