-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
One-to-many asset processing #21889
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
base: main
Are you sure you want to change the base?
One-to-many asset processing #21889
Conversation
08313c5 to
77091f1
Compare
JMS55
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very exciting! Left some feedback
- I really don't like the single vs multi naming. I think we need better naming.
- As someone not very familiar with the asset system, I don't quite get how the multi files get mapped to asset server concepts. Sub assets? Or?
- Do we support asset savers besides filesystems? How does this work with those?
| } | ||
| for (i, line) in bytes.lines().map(Result::unwrap).enumerate() { | ||
| let mut writer = writer_context | ||
| .write_multiple(Path::new(&format!("Line{i}.line"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write_multiple() in the loop, for writing a single item, feels kind of weird to me. Can we call it something else?
write_part, write_subasset, write_new_asset, ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point! I tried renaming write_single and write_multiple to write_full and write_partial respectively. The naming here really sucks. We shouldn't use subasset because that's already taken and it has a completely different meaning here.
I want to make sure that the "full" vs "partial" are like antonyms (like single vs multiple). Just to give the intuition that these can't be mixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little late but - what about write and write_many? Similar to how some rust standard library types have X and X_many - like get and get_many on collection types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would have the same problem as write_multiple: calling write_many in a loop feels wrong.
andriyDev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To respond to your questions:
- Good point! Discussed in a comment.
- Multi-files are just treated as individual assets. There is no requirement that subassets map to some multi-file. If the process wants to throw out the bytes of the original asset and write 100 copies of the Bee Movie script, that's totally up to it!
- The
Processtrait has no inherent relationship toAssetSaver, both before and after this PR. TheAssetSaveris used by the particularProcessimplementationLoadTransformAndSave, which makes it slightly more convenient to writeProcessimplementations. We could figure out a similar convenience for one-to-many, but I think we should leave that for another PR, since that seems a little tricky to get right, and I don't think we need it for a glTF processor.
| } | ||
| for (i, line) in bytes.lines().map(Result::unwrap).enumerate() { | ||
| let mut writer = writer_context | ||
| .write_multiple(Path::new(&format!("Line{i}.line"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point! I tried renaming write_single and write_multiple to write_full and write_partial respectively. The naming here really sucks. We shouldn't use subasset because that's already taken and it has a completely different meaning here.
I want to make sure that the "full" vs "partial" are like antonyms (like single vs multiple). Just to give the intuition that these can't be mixed.
7cf4977 to
c8f2933
Compare
How does this work for non-filesystem-based asset saver things?
After doing some more thinking, I think it would be better if What do you think? |
Asset sources currently have to act like a filesystem. They need to be able to have assets as well as directories. It's also important to note this only matters for the processed reader - not for the unprocessed reader. In practice, I expect the processed reader/writer will always be a local file system.
The problem with that is we still need to finalize the writer anyway (since we can no longer just return the meta, since there may be more than one), so the API will necessarily change. I'd prefer to make the writer explicit in that case rather than only fetching the writer on write, which is what would have to happen to support the API you're proposing. |
Can't we just make this happen on writer drop? |
|
No because we need the process to give us loader settings. That's really what finalization is for. |
JMS55
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving this for now, because I think having the infrastructure and functionality is more important than blocking on the user-facing API, but I really don't like the write_single / write_partial API.
Imo I would much rather either:
- User consumes the initial writer object, gets back a strongly-typed AssetWriter or MultiAssetWriter or something
- User only ever calls write_asset(), Bevy transparently detects when a second call is made and moves the first asset to a folder.
We almost have this.
This would mean we can't parallelize writes, since you need the first write to be complete before you can move it to the folder. So now all of your writes need to happen one at a time, which is sad. I also am not a fan of how implicit it is, especially if your process "accidentally" writes only a single asset. Like imagine a glTF that we want to split into multiple pieces: If there's only a single mesh in the glTF, it may only write one asset, and now whether you need to access it as |
|
Very fair, glad to see that you've thought this out much more than I have 😅 In that case my only remaining gripe is that write_partial feels like a poor name for it since the assets you write are unrelated to each other besides the fact that they come from the same processed asset, iiuc. |
NthTensor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I've been wanting this functionality for ages.
9ea597d to
02d6ca7
Compare
0e62b36 to
0882e7d
Compare
…cessing doesn't rerun if nothing has changed.
…nd "ghost meta files".
… an asset to prevent failing to process.
…y asset processing.
0882e7d to
ba4baa4
Compare
Objective
Solution
&mut WritertoProcess, instead we pass aWriterContext. Users then just callwrite_singleorwrite_multipleto decide whether they want a single file or multiple files. Once users are done writing, they callSingleWriter::finishorMultipleWriter::finish.Decomposed. This makes it more obvious to callers that they are querying for the wrong asset path.This means if you have an asset path like
path/to/my_asset.json, and theProcessproduces files "part1.thing", "part2.thing", and "dir/mesh.gltf", these assets will be accessible atpath/to/my_asset.json/part1.thing,path/to/my_asset.json/part2.thing, andpath/to/my_asset.json/dir/mesh.gltf. In essence, the asset in the unprocessed directory becomes a folder in the processed directory.Testing
asset_processingexample works and has been updated to include a "multiple" processor.