-
-
Notifications
You must be signed in to change notification settings - Fork 746
Remove usage of String in the asset manager #88
Comments
Shouldn't most asset strings have the 'key' only allocated once per app? If not then a flyweight string container could do that. That way you get a unique id comparison via taking the pointer of the string or the appropriate method in rust. |
I agree this is probably true, but it's forcing the user to use a string when they otherwise would not. That's really what I'm trying to point out.
This makes sense in the current implementation to me. |
Here's a pull request showing what I'm thinking. Any thoughts? I'd like to audit everything for this change. I'm happy to add some implementation of string based asset-management that's built ontop of this, if that's desired. My real goal is to allow myself a path for not using strings here. |
Could we make it a generic so that the user can decide? Another thought I had is making a build script that takes a resource file and bakes the strings into integers, similar to how android does it. The build script could generate a bunch of environment variables like
Or even a custom macro so we can omit the prefix:
Anyway, those are my thoughts. Don't know if we want to mess with build scripts. |
I like this, I think I can put a builder in front of this and make it general. |
Another thought: If we're going to deal with build scripts, we could just generate a "resources.rs" that contains all the resource constants. It would mean that all the |
One way to have both i64's and String is to with #90 have each function take id: T where T: Into<*Id> |
Another thought, maybe it makes sense to always support static str& but String explicitly separately? |
The asset manager has changed significantly since the creation of this issue and uses |
Looking up resources by a string is convenient but slow thanks to dynamic allocation. I believe this is why OpenGL uses integer handles, even if it they are less convenient to work with.
These string should be replaced with type-safe integer wrappers, or some other design to improve the performance of the asset manager.
https://github.com/amethyst/amethyst/blob/develop/src/context/src/asset_manager.rs#L41-L42
Consideration must be given to the current convenience though, especially if it is removed.
Maybe offering both versions make sense? What does everyone think?
The text was updated successfully, but these errors were encountered: