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

Change StorageMemoryUsage type from struct to enum #219

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

Conversation

notinmybackyaard
Copy link
Contributor

@leudz
As mentioned earlier, I attempted to convert the StorageMemoryUsage type into a trait. The first approach involved defining an Out type for each Storage type that implements the StorageMemoryUsage trait, allowing the memory_usage function to return an Option<Box> type. The second approach was to add StorageMemoryUsage as an associated trait to the Storage trait. The first approach felt overly verbose, and the second required too extensive code changes, so I opted for a simpler solution by handling it as an enum to enable more detailed information during debugging. If there are any suggested modifications, please let me know.

notinmybackyaard added a commit to Npixel-Eclipse/shipyard that referenced this pull request Feb 26, 2025
@leudz
Copy link
Owner

leudz commented Feb 27, 2025

Thanks for putting this together!
Sadly an enum cannot work. Users are free to add other storages that could be any type ('static + Storage is the minimum bound). This is done with the CustomStorageAccess trait.
Sorry.


What do you think of something like this?

pub trait MemoryUsage {
    type Out;

    fn detailed_memory_usage(&self) -> Self::Out;
}

SparseSet and Entities could implement this trait. Maybe in the future World and AllStorages too. For Unique it would have to be implemented on the views but I wouldn't bother (at least for now) since its size is fixed.
StorageMemoryUsage wouldn't change.
MemoryUsage would be public so users can call detailed_memory_usage directly.

Does this make sense? Do you think it would also be too verbose?

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

Successfully merging this pull request may close these issues.

2 participants