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

Fixes and improvements #183

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

Fixes and improvements #183

wants to merge 10 commits into from

Conversation

nrader95
Copy link
Contributor

@nrader95 nrader95 commented May 4, 2023

Basically what i already described in #178 plus readme update.

@nrader95
Copy link
Contributor Author

nrader95 commented May 4, 2023

Oh and, those changes are actually tested by me and my colleagues for some time, the reason commits marked as new is because i did a bit of clean up today before making this PR and basically rewritten commit history in my fork.

@alkboda
Copy link

alkboda commented Jun 19, 2023

I have no great desire to rewrite all my systems classes constructors to manually define false useBuffer parameter. Wiki clearly states rules how to work with entities within basic system ancestors Update method. See no reason to use worse performance case as default behavior. If I need safe context, then I set useBuffer manually. But that's my personal opinion.

@nrader95
Copy link
Contributor Author

The thing with no useBuffer param is that when it does go wrong, trying to detect it is a real pain. So chances are, you wont even know you need this safe context.
What's more, the potential perf gain doesnt even matter most of the time, because your system has to have a very light update to care about such overhead and do so in single-thread (useBuffer is not used in multi-thread, after all), on a great number of entities. That's the only situation where you might reasonably want to disable the buffering, but such systems are not that common in my experience. Unless you trying to write game engine of your own or something, with DefaultECS as the basis.

@alkboda
Copy link

alkboda commented Jun 26, 2023

Debugging ECS is really painful, totally agree on this. But buffering systems updates isn't only thing that matters. We're developed a bunch of instruments for debugging, and each time we're forced to think out something new(eg. last time I wrote wrapper for ReanOnlySpan which I return from EntitySet::GetEntities() and which tracks changes made to EntitySet while ReadOnlySpan is enumerated - that solves all my problems with incorrect Updates within my systems).
UseBuffer situations could be detected by switching default parameter value to true - if that solves the problem, then definitely some of your systems violates rules.

@nrader95
Copy link
Contributor Author

@Doraku So, any response?
Since you seem to be active again.

@Doraku
Copy link
Owner

Doraku commented Dec 2, 2024

On the debug stuff, I'll probably opt for a debug package or the nuget containing both release and debug dll and reference one or the other based on the configuration 🤔 I like the idea of better error reporting during development.

On the buffer default, I think it should be possible to keep it to false while still checking for incorrect usage in debug with extra code executing in the system so people are clearly notified of what they did wrong instead of just guessing like currently.

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