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

Changes SharedGridTraversalSystem accesibility from internal to public #5551

Merged
merged 2 commits into from
Dec 21, 2024

Conversation

MLGTASTICa
Copy link
Contributor

Lets people disable/enable this system at will with this change. Done mainly because this prevents anyone from implementing custom parenting/unparenting behaviour and limits everyone to grid-parenting preety much.

@metalgearsloth
Copy link
Contributor

Are they going to manage broadphases manually too

@MLGTASTICa
Copy link
Contributor Author

Are they going to manage broadphases manually too

Probably not.

@ElectroJr
Copy link
Member

In case it isn't clear, AFAIK the issue is that if an entity is far away from the grid, but still parented to it, then it will still be part of the entity's broadphase. Any system that tries to query entities in an area (physics, lighting, rendering, entity lookups, etc) first gets a list of grids in the area, then queries each of those grids. So if an entity is still parented to a grid despite being far enough away, from the grid, it will no longer be returned by queries and wont get rendered or whatever else is being done by the query.

So unless there's some system that handles expanding a grids "size" to account for entities that are parented to the grid despite not being on the grid (which I don't know if we'd even want to bother supporting), we kind of need any entity that moves off the grid to automatically get parented to the map.

I get that the system can be a PITA and it might be nice to have more control (e.g., the bool exists because it can be a PITA for tests). But if people just start completely disabling it they will probably just unknowingly introduce bugs. But if people are aware of the issues it will cause and still want to be able to disable it I think its fine to make it public, though there should be comments in the code explaining the issues.

@MLGTASTICa
Copy link
Contributor Author

In case it isn't clear, AFAIK the issue is that if an entity is far away from the grid, but still parented to it, then it will still be part of the entity's broadphase. Any system that tries to query entities in an area (physics, lighting, rendering, entity lookups, etc) first gets a list of grids in the area, then queries each of those grids. So if an entity is still parented to a grid despite being far enough away, from the grid, it will no longer be returned by queries and wont get rendered or whatever else is being done by the query.

So unless there's some system that handles expanding a grids "size" to account for entities that are parented to the grid despite not being on the grid (which I don't know if we'd even want to bother supporting), we kind of need any entity that moves off the grid to automatically get parented to the map.

I get that the system can be a PITA and it might be nice to have more control (e.g., the bool exists because it can be a PITA for tests). But if people just start completely disabling it they will probably just unknowingly introduce bugs. But if people are aware of the issues it will cause and still want to be able to disable it I think its fine to make it public, though there should be comments in the code explaining the issues.

Thank you for explaining the issue. I thought that entity lookups used map chunks to check for entity positions. Ill add a comment specifying the issue.

@metalgearsloth metalgearsloth merged commit 6247be2 into space-wizards:master Dec 21, 2024
3 checks passed
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.

3 participants