Feature flags + Real example features #591

2025-02-09T16:21:54Z

NQNStudios commented

2025-02-09 16:21:54 +00:00

(Migrated from github.com)

Here's a 2nd attempt at the feature flags system.

Scenarios contain a string map of feature flags. The flag names are the keys, and flag versions are the values, so a typical value might be "fixed" for bug fixes or for evolving features, "V1", "V2", etc.
The game has a map of flags to lists of supported versions. The game can therefore signal that it supports a legacy behavior for a given feature flag. The last version in the list is considered to be this build version's default behavior.
When launching a scenario, we check to make sure the game supports the scenario's required versions of its feature flags.
When launching a replay, we make sure the game supports the feature flags that the version of the game that made the recording did.

Here's a 2nd attempt at the feature flags system. * Scenarios contain a string map of feature flags. The flag names are the keys, and flag *versions* are the values, so a typical value might be "fixed" for bug fixes or for evolving features, "V1", "V2", etc. * The game has a map of flags to *lists* of supported versions. The game can therefore signal that it supports a legacy behavior for a given feature flag. The last version in the list is considered to be this build version's default behavior. * When launching a scenario, we check to make sure the game supports the scenario's required versions of its feature flags. * When launching a replay, we make sure the game supports the feature flags that the version of the game that made the recording did. Fix #555 Fix #146 Fix #635

CelticMinstrel commented

2025-02-09 16:34:52 +00:00

(Migrated from github.com)

Okay… this still feels like something is missing, though that might be inevitable since currently no feature flags exist.

With this system, can the existing "legacy" flag be replaced by one or more feature flags? We don't have to actually replace it, but it seems like a good test of whether the feature flag system does what it needs to do.

Okay… this still feels like something is missing, though that might be inevitable since currently no feature flags exist. With this system, can the existing "legacy" flag be replaced by one or more feature flags? We don't have to actually replace it, but it seems like a good test of whether the feature flag system does what it needs to do.

NQNStudios commented

2025-02-09 16:34:54 +00:00

(Migrated from github.com)

There's something I still need to fix here....

NQNStudios commented

2025-02-09 18:06:17 +00:00

(Migrated from github.com)

Ok I had realized I made get_feature_version() less than useful.

I have clarified its purpose: The game will call get_feature_version() to find out which version of a feature should be active. So if a scenario requests an outdated feature, the game will be able to check and find that out easily. For flags that have nothing to do with scenarios, such as bugfixes, this function returns the behavior that the replay used.

Ok I had realized I made get_feature_version() less than useful. I have clarified its purpose: The game will call `get_feature_version()` to find out which version of a feature should be active. So if a scenario requests an outdated feature, the game will be able to check and find that out easily. For flags that have nothing to do with scenarios, such as bugfixes, this function returns the behavior that the replay used.

CelticMinstrel commented

2025-02-09 22:35:57 +00:00

(Migrated from github.com)

Did you miss my previous comment in this thread? You commented at the exact same time, so maybe it slipped through your notice…

NQNStudios commented

2025-02-09 22:45:05 +00:00

(Migrated from github.com)

I saw it. Yeah, I'm planning to write up a little example -- just also working on other issues and PRs.

NQNStudios commented

2025-02-16 14:49:31 +00:00

(Migrated from github.com)

Just rebased onto master. Today, I'm going to implement some features and fixes as part of this PR -- that way, I can make sure the system does what I want it to, and you can see concrete examples of that vision.

NQNStudios commented

2025-02-17 16:59:52 +00:00

(Migrated from github.com)

Ok. I've added 2 features which demonstrate how I see feature flags being useful:

You can choose to have the picture in a dialog node be animated, if the graphic supports it. This is a Scenario feature flag -- old scenarios will not animate their dialog pictures under any circumstances, but new scenarios can do it, and will signal to the game that this needs to be supported. (This applies to animated terrain graphics, monster graphics, booms (fixing #146), and missiles.)
In debug mode, you can give a PC 4 more AP by selecting them if they already acted that turn. This is a Game feature flag -- It only affects game behavior, and does so regardless of needing the scenario to signal support. The purpose of the flag is so that replays recording the old behavior will not become broken by using the new behavior and throwing everything off.

Since we don't have a stable release, technically we could make these changes without the flags--especially the 2nd one, because replay files are pretty ephemeral right now, we're not maintaining many of them in the actual repo. But these 2 features show how, if we did have a stable release, we could add new features without breaking things.

Ok. I've added 2 features which demonstrate how I see feature flags being useful: 1. You can choose to have the picture in a dialog node be animated, if the graphic supports it. This is a **Scenario feature flag** -- old scenarios will not animate their dialog pictures under any circumstances, but new scenarios can do it, and will signal to the game that this needs to be supported. (This applies to animated terrain graphics, monster graphics, booms (fixing #146), and missiles.) 2. In debug mode, you can give a PC 4 more AP by selecting them if they already acted that turn. This is a **Game feature flag** -- It only affects game behavior, and does so regardless of needing the scenario to signal support. The purpose of the flag is so that replays recording the old behavior will not become broken by using the new behavior and throwing everything off. Since we don't have a stable release, technically we could make these changes without the flags--especially the 2nd one, because replay files are pretty ephemeral right now, we're not maintaining many of them in the actual repo. But these 2 features show how, if we did have a stable release, we could add new features without breaking things.

NQNStudios commented

2025-02-17 17:03:27 +00:00

(Migrated from github.com)

I just want to get the foundation of feature flags into the code before 2.0 beta. One thing that's definitely needed, but can wait for a later milestone, is the ability to modify/update a scenario's feature flags in the editor.

CelticMinstrel commented

2025-02-17 20:07:39 +00:00

(Migrated from github.com)

The scenario feature flag example still doesn't quite make sense to me. Based on my understanding of how it should work, the logic should amount to "if this feature flag is not present, then don't do anything". Or, to put it another way, "if this feature flag is present, then do the new feature". To me, that implies that the result of has_feature() should have been passed as the anim_pict argument. As it currently stands, that argument appears to do nothing.

I think the overall result technically does fit my understand as described above, but it is done in a confusing way.

The scenario feature flag example still doesn't quite make sense to me. Based on my understanding of how it should work, the logic should amount to "if this feature flag is not present, then don't do anything". Or, to put it another way, "if this feature flag is present, then do the new feature". To me, that implies that the result of `has_feature()` should have been passed as the `anim_pict` argument. As it currently stands, that argument appears to do nothing. I think the overall result technically does fit my understand as described above, but it is done in a confusing way.

NQNStudios commented

2025-02-18 15:35:36 +00:00

(Migrated from github.com)

Oh--I remember adding that argument purely for its default value to be false. This wasn't to keep legacy scenarios from using the feature, it was to keep unrelated callers of the function to behave the same way they used to. If I had added only the two integer arguments with default values, then every caller would suddenly be animating things.

Alternatively, I could have made an overload of the function.

Oh--I remember adding that argument purely for its default value to be false. This wasn't to keep legacy scenarios from using the feature, it was to keep *unrelated callers* of the function to behave the same way they used to. If I had added only the two integer arguments with default values, then every caller would suddenly be animating things. Alternatively, I could have made an overload of the function.

NQNStudios commented

2025-02-19 18:03:39 +00:00

(Migrated from github.com)

I added another replay-related feature flag: the Give Item debug action now tries to give to the active PC first.

To test that it would still fall through to give to the first PC with empty space, I added a debug action that stuffs the active PC's inventory with weightless junk items.

I added another replay-related feature flag: the Give Item debug action now tries to give to the active PC first. To test that it would still fall through to give to the first PC with empty space, I added a debug action that stuffs the active PC's inventory with weightless junk items.

CelticMinstrel (Migrated from github.com) reviewed 2025-02-19 18:55:44 +00:00

src/game/boe.actions.cpp

						
				@@ -2005,13 +2007,41 @@ void debug_give_item() {

					int i = get_num_response(0, univ.scenario.scen_items.size()-1, "Which item?", item_names);

CelticMinstrel (Migrated from github.com) commented

2025-02-19 18:55:43 +00:00

I have two comments on this feature.

This seems like a good place to use the eItemPreset constructor of cItem.
It's fine using weightless objects, but I feel like there may also be something to be said for using objects that are ridiculously heavy… though I don't remember if BoE actually gives any penalties for being overburdened in weight.

I have two comments on this feature. 1. This seems like a good place to use the eItemPreset constructor of cItem. 2. It's fine using weightless objects, but I feel like there may also be something to be said for using objects that are ridiculously heavy… though I don't remember if BoE actually gives any penalties for being overburdened in weight.

NQNStudios (Migrated from github.com) reviewed 2025-02-19 19:03:59 +00:00

src/game/boe.actions.cpp

						
				@@ -2005,13 +2007,41 @@ void debug_give_item() {

					int i = get_num_response(0, univ.scenario.scen_items.size()-1, "Which item?", item_names);

NQNStudios (Migrated from github.com) commented

2025-02-19 19:03:59 +00:00

I went with weightless thinking it was necessary so the weight limit wouldn't stop the function from filling every slot. Now I see there's a flag GIVE_ALLOW_OVERLOAD that can do that.

NQNStudios (Migrated from github.com) reviewed 2025-02-19 19:21:23 +00:00

src/game/boe.actions.cpp

						
				@@ -2005,13 +2007,41 @@ void debug_give_item() {

					int i = get_num_response(0, univ.scenario.scen_items.size()-1, "Which item?", item_names);

NQNStudios (Migrated from github.com) commented

2025-02-19 19:21:23 +00:00

Now they're heavy and it's a preset.

CelticMinstrel commented

2025-02-20 03:29:40 +00:00

(Migrated from github.com)

The above commit is squashed from the first two commits in this PR, in other words, the actual core of the feature. The remaining commits are left as examples of how feature flags might work, until such time as GitHub decides to collect the garbage.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: openboe/oboe#591

				`@@ -2005,13 +2007,41 @@ void debug_give_item() {`
				`int i = get_num_response(0, univ.scenario.scen_items.size()-1, "Which item?", item_names);`