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

feat: User-defined assets support (ScriptableObject like data types and more) #2527

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Eideren
Copy link
Collaborator

@Eideren Eideren commented Nov 21, 2024

PR Details

WIP right now - it works but barely. I feel like most of the logic shouldn't be where I ended up shoving it, would need someone aware of how quantum, the assembly reloader and the undo system work to check over this one.

To create an asset, see this page https://github.com/stride3d/stride-docs/blob/master/en/manual/scripts/custom-assets.md or @manio143 's https://github.com/manio143/StrideCustomAsset

Related Issue

#30

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 change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@Eideren Eideren marked this pull request as draft November 21, 2024 17:08
Copy link
Member

@manio143 manio143 left a comment

Choose a reason for hiding this comment

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

Good job on making this one work!

package.Assets.Add(newAssetItem);
AssetItem = newAssetItem;
PropertyGraph?.Dispose();
Session.GraphContainer.UnregisterGraph(assetItem.Id);
Copy link
Member

Choose a reason for hiding this comment

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

I think there's an implicit assumption here that previous asset item Id is equal to the new one. Would be good to verify it explicitly (at least with Debug.Assert).

@Eideren Eideren marked this pull request as ready for review December 7, 2024 16:20
@Eideren
Copy link
Collaborator Author

Eideren commented Dec 7, 2024

Cleaned up as much as I could, still a fair amount of logic that might be fragile, but I'm at a point were things are just too obtuse to investigate further.

@xen2
Copy link
Member

xen2 commented Dec 9, 2024

There is already a mechanism to handle type reloading, but it works only if it's inside an asset (as part of a property, member or list), not for the top-level asset itself.

I am currently testing/checking which option would make more sense:

  • Handle specifically the top-level asset reloading for this use case (similar to your current approach). However we will need to add support for undo/redo, fix references, etc.
  • Allow the other mechanism to work on top-level item (downside: AssetViewModel.Asset might become null, I think it might break a lot of stuff)
  • Use IUnloadable which is what we do when a top-level type can't be initially loaded.

Another important point is to handle failed/unloadable assets just like we do for properties (either due to assembly not compiling or any other Yaml mismatch) so I will try to check which of those strategy fit better considering this as well.

@xen2
Copy link
Member

xen2 commented Dec 11, 2024

I had to dig a bit deeper than expected because I ran into a situation that was not working properly (unrelated to your changes for custom asset).

So I spent some time to understand the various quantum nodes/reloading/propagator system and could finally fix this bug.

I did some tests and now it seems I can get proper reload for assets, components and references (tested on https://github.com/manio143/StrideCustomAsset).

Also, it should be not so difficult to add support to "unloadable assets" (just like we do for unloadable components/part of assets, this was just not possible at the top-level) since it would work the same way as we do for reload.

You can give it a try there:
https://github.com/xen2/stride/tree/improved_asset_reload
(I might still clean/polish/rewrite commits, and add support for unloaded assets before making final PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants