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

Some issues of framework #178

Open
nrader95 opened this issue Feb 9, 2023 · 4 comments
Open

Some issues of framework #178

nrader95 opened this issue Feb 9, 2023 · 4 comments

Comments

@nrader95
Copy link
Contributor

nrader95 commented Feb 9, 2023

So we are using this framework for quite some time now, and there there a few things that worth pointing out i my opinion.

  1. useBufer on systems being false by default. Great way to shoot yourself in the foot. Even if you DO know what its for and when it should be used, its still totally possibly to initially write system that doesn't require it, then make some changes to system that change that but forget to change constructor. Worse yet, if you set something on entity after dispose in such a system, errors will probably start to pop up in places totally unrelated to the problem since you basically set component on totally different entity. Enjoy your mysterious bugs. Not to mention that performance gain from useBuffer often is not that big anyway.
    Happened a few times in my project and it was not fun experience trying to debug it. (And looking at Entity Dispose Within a System #150 and Issue when disposing an entity from Update(TState state, in Entity entity) #176, i'm not the only one who had problems with it. )

  2. Entity.IsAlive
    It throws exception sometimes after trimming the world. Which is not something you probably expect from a property specifically made to keep watch for broken stuff.
    Also if game code relies to entities stored somewhere and not directly from entitySet then this property suddenly becomes pretty regular thing everywhere, which then makes its performance important. My solution (in fork) was to make lightweight version of it for checking dispose-state only (entity version match). Wouldn't hurt having something like this here in upstream too, i guess.

  3. Entity.Get error messages
    Not sure on this one in regards to addition it here in upstream, since Get's are supposed to be fast and additional checks for whether exception should be thrown is a overhead, but currently figuring what went wrong in release builds can be tricky sometimes, as lines get emitted from stack-traces. Especially when its not you getting an error and there is no way to just go and re-test it in debug environment.

@Doraku
Copy link
Owner

Doraku commented Feb 13, 2023

  1. You think useBuffer should be true by default then? Normally bad usage should be picked up by the analyzer but I now there is some cases not detected yet. I have some ongoing changes not pushed yet but I can't find the time to finish it >_<
  2. Oh that's a big miss of me, I didn't pay attention when adding the TrimExcess I'll fix that thank for reporting it.
  3. Someone suggested a version with safety check incorporated in the lib (to get better errors message in case that occurs). I'm probably going to go this road, creating a DefaultEcs.Debug package or something to better help find those kind of issues. Currently I'm trying to split DefaultEcs.Serialization from the main package so it'll be the big next refacto.

Thanks for reporting all that :)

@nrader95
Copy link
Contributor Author

  1. You think useBuffer should be true by default then? Normally bad usage should be picked up by the analyzer but I now there is some cases not detected yet. I have some ongoing changes not pushed yet but I can't find the time to finish it

The thing here is that not everyone is running the analyzers in the first place. So making potentially risky default behaviors and then pray analyzer catch the misuse is probably not the best idea.

2. Oh that's a big miss of me, I didn't pay attention when adding the `TrimExcess` I'll fix that thank for reporting it.

Lightweight version would be nice too. I mean, the moment you start to take entities from outside of EntitySet output is when IsAlive stops being a debug thing, and thus, its performance starts to matter.

3. Someone suggested a version with safety check incorporated in the lib (to get better errors message in case that occurs). I'm probably going to go this road, creating a DefaultEcs.Debug package or something to better help find those kind of issues. Currently I'm trying to split DefaultEcs.Serialization from the main package so it'll be the big next refacto.

Well, the whole idea is to receive adequate errors when you do not expect said errors. To give you an example of why, the demo of the game i'm working on is now on steam, some people encounter bugs.... and i have no clue what's that about, because its not a debug build, there is no lines specified in the logs... so, when errors happen most of the time i have no clue where to even start.
I mean, debug package wouldnt hurt, i guess, but i imagined something more primitive that will make it into release builds as well.

@Doraku
Copy link
Owner

Doraku commented Feb 15, 2023

falling into the pit of success instead of favoring the 5% edge case hum, it's probably the saner option.

the demo of the game i'm working on is now on steam, some people encounter bugs.... and i have no clue what's that about, because its not a debug build

ouch yeah for something as complex as a video game it's difficult to expect everything to be catched during development. Maybe losing some cycles to return an easy to track exception is wiser...

@nrader95
Copy link
Contributor Author

nrader95 commented May 4, 2023

Also i think you should probably drop support for .NET Standard 1.1

The reason is quite simple. Old projects that may still use it probably wont adopt it, since ECS framework is a core thing, you plug it in at the start of development, not after you already released your game or close to it.

And new projects even in worst-case scenario will probably have access to at least .NET Standard 2.0, since they will most probably use newer engines, or write their own.
Last Unity version that didnt support 2.0 is somewhere around 2017, and is no longer supported, for example.

Dropping it, instead, will allow to upgrade some things, like runners.

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