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

Garbage collection for Archetypes #277

Open
CGamesPlay opened this issue Jan 16, 2023 · 3 comments
Open

Garbage collection for Archetypes #277

CGamesPlay opened this issue Jan 16, 2023 · 3 comments

Comments

@CGamesPlay
Copy link

Hello! I stumbled upon this library looking for a good array-of-structs ECS system and I've been working with the for the past month or so in my game. I'm building a renderer using React, but I am not using miniplex-react (frankly, it doesn't seem to provide anything I would use that isn't very easy to implement myself).

I want to be able to render a list of entities matching a criteria, and I would like to be able to use an Archetype for this, however this causes a memory leak. I have a Location component which has a section field, and my React component should render all entities which have a given section. That's easy:

export function useEntities<T>(bucket: ArchetypeBucket<T>): T[] {
  const [_, forceUpdate] = React.useState({});

  React.useEffect(() => {
    const rerender = () => forceUpdate({});
    bucket.onEntityAdded.add(rerender);
    bucket.onEntityRemoved.add(rerender);

    return () => {
      bucket.onEntityAdded.remove(rerender);
      bucket.onEntityRemoved.remove(rerender);
    };
  }, [bucket]);

  return [...bucket];
}

function MyList({ section }) {
  const entities = useEntities(React.useMemo(
    () => world.with("section").where(e => e.location.section === section),
    [section]
  ));
  return (<div>...</div>);
}

This has a memory leak, since it creates a new Archetype every time the section prop changes, but Archetypes are never removed. The feature request is simple: once the last listener for a Bucket is removed, the Bucket should remove its own listeners. If future listeners for the bucket are added, the Bucket should reattach its own listeners and recalculate its entities.

I know I can create a global cache of every section and look up in that cache when rendering my component... but a cache without expiration is still just a memory leak.

Obviously this isn't critical, but since you're changing the API for 2.0 I figured now was an appropriate time to raise this. Thanks for continuing work on this library!

@hmans
Copy link
Owner

hmans commented Jan 16, 2023

Hi, and thanks for submitting this!

First of all, which version of Miniplex are you using exactly? Your code looks like you're using one of the 2.0 betas — which one?

To be entirely fair, I'm not sure if there really is a memory leak, or if there is one, that it's because of the archetype creation. A call to world.with("section") will be idempotent, only creating the actual archetype on its first call; on subsequent calls, the memoized archetype will be returned. archetype.where(predicate) won't even create any new archetype instances, but just a temporary iterator that allows you to iterate through archetype, with the predicate function applied.

If for some reason multiple instances of the same archetype are being created, that would be a bug. May I ask what steps you took to identify this memory leak? Thanks!

@hmans
Copy link
Owner

hmans commented Jan 16, 2023

Addendum: I'm honestly surprised that the code example you've posted here works at all, considering you're creating an iterator using where and passing that to your version of useEntities(), which will try to call .onEntityAdded et al on it, which should raise an error because those events don't exist on the iterator object where returns.

I'm assuming you're using 2.0b3 — please let me know if this is wrong.

The implementation of where is here, for reference.

Heads-up: because the nature of how where works has proven a bit iffy to explain to people, it'll be removed in the next beta. If things go as planned, the next beta will also come with a significant reduction in API surface and implementation complexity (but it'll be a breaking change, so be ready.) See #275 for details.

@CGamesPlay
Copy link
Author

CGamesPlay commented Jan 16, 2023

Sorry, I should have mentioned. I am using ^2.0.0-beta.3, which is apparently [email protected]. Also, the code snippet in my original is what I'd like to do, rather than what I am presently doing (which is using a normal archetype and filtering manually). 🫢 Given that you're saying it wouldn't work, well, it wouldn't be a leak either, I guess!

What I really want is to have a component that only rerenders when the entities selected by the predicate change. My actual code presently caches the world.with("section") globally, which is advised in the current README, and then as I mentioned I filter out other entities in my render method. This causes unnecessarily component renders of the list which I was hoping to avoid.

If you're moving towards the API you describe in #275, it would be awesome to have something like this (again, this is just pseudocode):

function MyList({ section }) {
  const entities = useQuery(world, React.useCallback(
    q => q.with("location").where(e => e.location.section === section),
    [section]
  ));
  return (<div>...</div>);
}

If it's possible to create such a useQuery hook, it would greatly reduce my number of rerenders.

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

No branches or pull requests

2 participants