beatsaber-http-status Add energy support.

To make my issue #4 happy, I added some info around player HP, which the game calls energy.

This adds:

an energy property on the performance object
a new event energyChange, which acts the same as other similar events.

we need more power

Oct 17 '18 04:10 SplitPixl

First, some complaining about your commit/PR:

You made an unrelated documentation edit in a PR adding a feature - it should be a separate commit and a separate PR. Why? Because that way it can be immediately merged in, even if there's an issue with rest of the PR.
Your brackets don't match the rest of the code (you put them in the next line as opposed to same line like the rest of the code).
You committed a modified .sln file which once again shouldn't be a thing considering this is a PR adding a new feature.

As for the implementation of the energyChanged event itself, the reason I haven't implemented it myself yet is that some events imply a change in the energy meter. Cutting a note successfully always adds 0.05 energy, missing a note subtracts 0.1 energy, cutting a note subtracts 0.15 energy, etc. Those changes should be reflected in the noteCut, noteMissed, bombCut and other events already when they are fired. I feel like they shouldn't be followed up by a separate event that just changes the energy field - it's less efficient (two events fired with almost the same data) and can cause confusion if someone happens to expect that change in those events (which I know I personally would).

However, trying to implement it raises a potential problem caused by the event listeners potentially being called in the wrong order. It appears that event listeners should be called in the order in which they were registered, but there's no guarantee that we will always receive those events before or after the other event listeners in the game (the order in which those events are registered could change or simply be different than what we expect - needs testing). In particular, I'm talking here about the event handlers in the GameEnergyCounter. That class is responsible for modifying the energy count whenever the player performs some action and uses the same events we use to change the energy meter.

For example, if we were to update the energy meter in the note cut event handler and suppress the next energyChanged event, there's a chance that the GameEnergyCounter didn't yet receive the event and so the field wouldn't have been updated in the noteCut event and the energyChanged event would've never been fired. The other possibility is that GameEnergyCounter already received the event and that everything would work just fine. This is something that would require testing and could prove to be unreliable. Unfortunately, I didn't test it myself yet as it's not easy for me to do with my current setup.

Always firing the energyChanged event instead and never guaranteeing that the energy field will be updated with the correct value is a workaround for that issue, but I don't really like this solution as it's not very user friendly and will seem unpredictable.

Next problem is that you passed ChangedProperties.Beatmap to EmitStatusUpdate right here. That will send an event with only the beatmap part of the status object. I'm honestly quite surprised you didn't catch that during testing - I suggest checking the output in the future. The correct value in this case is ChangedProperties.Performance. For reference, see ChangedProperties and OnStatusChange.

I recommend moving the documentation fixes to a separate PR so I can merge them in. As for the energyChanged event itself, we can either use this simple implementation for now and work on improving it later or improve it now before the next release. The current solution would require some changes to the documentation to make it clear that the performance.energy field is only updated for the energyChanged event. It will also possibly require code from other repositories to be updated when the improved version is implemented, which is why I'm not really in favor of just merging the current solution in - the future versions won't be backwards compatible.

Oct 17 '18 14:10 opl-

It also appears that you don't set a default value for the energy field when the song starts unless no fail is enabled, which could result in the songStart event being fired with a random amount of energy.

Oct 17 '18 14:10 opl-

Also also, looking at the code (without actually testing it) it appears that energy is still modified when no fail is enabled. The game simply doesn't trigger the gameEnergyDidReach0Event event. Since your code doesn't have any checks for that (which it shouldn't), the energyChanged event will still be fired even if no fail is enabled, which clashes with what you wrote in the documentation.

The amount of energy should always be read from the GameEnergyCounter, even when the song is just starting. It should also always be updated with the value the game thinks it is: it's up to the software using that data to check if no fail is enabled and handle the energy field in a suitable way. Why shouldn't we expose that data if it's available?

Oct 17 '18 14:10 opl-

Here's a list of actions that affect the energy meter that need to be considered:

Action	Cause of energy change	Energy change	Notes
Cutting a note correctly	`BeatmapObjectSpawnController.noteWasCutEvent`	+0.05
Cutting a note incorrectly	`BeatmapObjectSpawnController.noteWasCutEvent`	-0.1
Cutting a bomb	`BeatmapObjectSpawnController.noteWasCutEvent`	-0.15
Missing a note	`BeatmapObjectSpawnController.noteWasMissedEvent`	-0.1
Player's head in an obstacle	`GameEnergyCounter.Update`	-0.1/s	Changed every tick by `delta * -0.1`

Oct 17 '18 15:10 opl-

I'm thinking about the implementation of the energyChange event, it makes sense to leave energyChange implied in the cases of noteCut, noteMissed, and bombCut. But for the obstacleEnter event I am wondering if you would want to emit an event when the energy changes while you are inside the obstacle, or leave that for clients to determine.

Oct 17 '18 16:10 SplitPixl

Also, maybe keep the current behavior of energy when noFail is enabled, and fix my documentation to reflect it's behavior?

Both of these really come down to personal preference on how you want this plugin to behave.

Oct 17 '18 16:10 SplitPixl

I'd say obstacleEnter probably shouldn't affect the energy, since it should be fired when the player's head enters the obstacle, not after it's been in it for a tick or two. obstacleExit on the other hand should probably take into account the last tick during which the player's head was in it, since after that the energy won't be affected by that factor any more.

And yes, the current behavior should be kept and the documentation changed to make it clear that the client will need to check for no fail if it doesn't want to use energy when it's enabled.

Oct 17 '18 17:10 opl-

Looking at CheckHeadInObstacle, maybe there could be like a obstacleEnergyChange event? It might need to be ratelimited to not spam the client.

Oct 17 '18 18:10 SplitPixl

CheckHeadInObstacle should admittedly be moved to the Update method, as the TODO suggests. It's currently called whenever the score changes, which is a bit sketchy, but shouldn't affect anything when I fix it (which I probably should). Rate limiting the event and specifying the reason for the change sounds like a good idea. There aren't any other causes for the energy change in the base game (and the only mod that could be touching that that I can think of is the no fail on fail mod). If they ever add passive energy regeneration it should hopefully be rather easily detected too and get it's own dedicated event.

Oct 17 '18 18:10 opl-

When I can test the new code on master, I'll merge it in to here and see if I can make a ratelimited obstacleEnergyChange event.

Oct 17 '18 21:10 SplitPixl

I'm thinking that the obstacleEnergyChange event would probably be better as it's own PR.

Oct 18 '18 18:10 SplitPixl

Hey, is there anything else that needs to happen before this PR can be merged?

Oct 21 '18 23:10 SplitPixl

Hey, is there anything else that needs to happen before this PR can be merged?

Oct 21 '18 23:10 SplitPixl