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

Feature flags in the game and scenarios #584

Closed
wants to merge 5 commits into from

Conversation

NQNStudios
Copy link
Collaborator

I've implemented my idea for the feature flags system. The game has feature flags which are recorded and replayed so we can keep legacy behavior the same in replays. As an example, I've fixed the shirt bug mentioned in #245 in a way that should keep old replay behavior the same (not that we have any replays that had the shirt bug in them...)

I also gave scenarios feature flags which we'll eventually be able to use to detect in the editor whether anything needs to be migrated to a new format to leverage a new feature.

Fix #555

@josefwk
Copy link
Contributor

josefwk commented Feb 9, 2025

Recommend editing item.cpp to display the correct defense value as well. Damage calculation is correct but the item pickup window still shows that a Shirt will Block 1-1 damage.

@CelticMinstrel
Copy link
Member

It seems like something is missing here. I don't see any code that initializes the feature flags when starting a new scenario.

@NQNStudios
Copy link
Collaborator Author

NQNStudios commented Feb 9, 2025

The scenario's feature flags are added to the scenario by the editor when it creates it.

https://github.com/calref/cboe/pull/584/files#diff-e01fb704d3efd4e5ba7dd3cad2b5599bde269e0bde79ab25e9296d8c52ff34a6R2957

@CelticMinstrel
Copy link
Member

But the game only uses the global feature flags. The scenario feature flags have no effect. Plus, setting the feature flags to empty in a new scenario doesn't seem correct.

Or to put it another way, it looks like there is nothing to make the feature flags actually do the thing they're supposed to do, namely making the shirt block 1 damage if you play a legacy scenario and 0 damage if you play a newly-created scenario.

@NQNStudios
Copy link
Collaborator Author

Oh, I wasn't concerned with keeping shirt defense at 1 for legacy scenarios. That just seems like a bug.

The feature flag is there for the hypothetical scenario where a replay might exist where the bug is pivotal. (In this case, I don't think one does.)

@NQNStudios
Copy link
Collaborator Author

I care more about the feature flag infrastructure than the shirt defense thing.

@CelticMinstrel
Copy link
Member

Oh, I wasn't concerned with keeping shirt defense at 1 for legacy scenarios. That just seems like a bug.

It does seem like a bug, but the shirt defense thing also doesn't seem like a good example of when you'd want a feature flag. Isn't the feature flag system supposed to be a generalization of the scenario "legacy" flag? With this implementation it definitely doesn't seem to be doing its job. Maybe we have different ideas of what it's supposed to do though. But, even considering that, it doesn't make any sense to add scenario feature flags that do nothing at all.

@NQNStudios
Copy link
Collaborator Author

The scenario editor and the game will both be able to check for scenario feature flags when we do need to add any.

@NQNStudios
Copy link
Collaborator Author

I'm going to separate out the armor fix as its own PR.

@NQNStudios
Copy link
Collaborator Author

Retooling this for a different PR.

@NQNStudios NQNStudios closed this Feb 9, 2025
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.

Feature flags
3 participants