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

Make the engine oblivious of the data passed between map reloads #2066 #21

Open
wants to merge 1 commit into
base: community
Choose a base branch
from

Conversation

teeworlds-mirror
Copy link
Collaborator

@ChillerDragon
Copy link
Member

It is not a bug fix or small tweak. But I would still consider it in scope for teeworlds-community. I would need quite some time to fully audit the code and understand all of the possible pit falls. But in general I do like the concept.

One thing to think about is that if we merge this in teeworlds-community and it will not be merged upstream that is quite some code diff then. It is touching an enum.

https://github.com/teeworlds-community/teeworlds/pull/21/files#diff-288861359e1ec0c2882b45578dbbcc73138508b5be3ef3e24f83e45dbdf1a88aL95-R96

So this would cause teeworlds-community's STATE_READY and STATE_INGAME to have different values than in teeworlds.

Also virtual functions is what I would consider the more user (as in mod developer) facing parts of the code.

https://github.com/teeworlds-community/teeworlds/pull/21/files#diff-6ff2b5bce28988049499ccee190cacc40028157e6400adfacc2d4600cbbed0ddL88-R112

If the signature of virtual void OnClientConnected(int ClientID, bool AsSpec) = 0; is different in teeworlds-community than in teeworlds. It becomes less of a drop in replacement.

Not sure if all of these changes are worth it. Does the user of teeworlds-community get enough out of this? I could see how some mod devs would appreciate a clean api to persist data across map reloads.

Also while this increases the code diff to teeworlds it would reduce it to ddnet. In ddnet this change got merged and all ddnet devs would be used to that api already.

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.

4 participants