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

Fix hunter background action names #9142

Draft
wants to merge 2 commits into
base: thewarwithin
Choose a base branch
from

Conversation

scamille
Copy link
Member

@scamille scamille commented Aug 1, 2024

Stumbled upon this when fixing compiler warnings. The fix is based on my understanding of how the get_background_action functionality is supposed to work.

Should be reviewed by some actual hunters though ;)

@scamille scamille requested a review from a team August 1, 2024 20:47
Copy link
Contributor

@Pewtro Pewtro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think we've had any issues with the background actions, what is this meant to fix?
And looks like it's breaking CI

@scamille
Copy link
Member Author

scamille commented Aug 2, 2024

The idea of the get_background_actions - as far as I understand it - is that you only have a single instance per player, based on the action name.

The issue here is that the background actions aren't created with the correct name - thus they are recreated each time get_background_action is called.

In the end this is mostly a object optimization.

@scamille
Copy link
Member Author

scamille commented Aug 2, 2024

In the end the question is whether most of these background actions - let's pick serpent_sting_explosive_venom should actually be separate from their base actions like serpent_sting or whether they should share the name, e.g. for expressions.

If the actions should be truly separate, then constructing them with a correct name is indeed required.

@Pewtro
Copy link
Contributor

Pewtro commented Aug 3, 2024

They should not be separate, they should share name

@Pewtro
Copy link
Contributor

Pewtro commented Aug 3, 2024

This can be merged once the CI passes, it's currently causing either dot.serpent_sting or action.serpent_sting to fail

@scamille
Copy link
Member Author

scamille commented Aug 4, 2024

They should not be separate, they should share name

Ok thanks. So if they indeed should share the name, but ideally still be a single global action unique to the player, this just needs something different than get_background_action that does lookup based on a unique name.

@scamille scamille force-pushed the bug/fix-hunter-background-action-names branch 3 times, most recently from 58a20b2 to a5ee04d Compare August 4, 2024 16:02
In the hunter module, get_background_action is often used with the presumable expectation to only create a single action per hunter (purely as an optimization). Since the background actions do not have pass on their constructor name to the action::name, that is not the case and the action get's constructed every time anyway. At the same time, the constructor name are just silently hidden instead to conform to the get_background_action.

Add an assert to the function, remove these non-intuitive usages.
@scamille scamille force-pushed the bug/fix-hunter-background-action-names branch from a5ee04d to fade35b Compare August 4, 2024 16:05
@Jayezi
Copy link
Contributor

Jayezi commented Aug 4, 2024

think it just needs to make sure any of the actions that got looked up with get_background_action before gets background set to true in case it was relying on the lookup to do that before, I see a few at glance that would need it

@nuoHep
Copy link
Member

nuoHep commented Aug 8, 2024

The idea was (as implemented in 4cb5c7c; the implementation did not change afaict, notwithstanding changes outside of the class module itself):

  • background action objects have name in the constructor and always pass it through to the base object constructor (up to action_t ctor)
  • get_background_action() gets called with a name "unique" for this background action, the constructed action gets constructed with this name so it can be looked up later

This is mostly a memory optimization (has negligible positive impact on perf) to share these "background" actions between different instances of the main action because of the way apl & stuff is handled in simc (each action line in the apl results in an action object).

I don't know why the actions used with get_background_action() don't propagate their names now, but "by design" (of get_background_action()) they should.

@Jayezi
Copy link
Contributor

Jayezi commented Aug 11, 2024

Main culprit that comes to mind is explosive shot, there's like a dozen different ways it shows up with different qualities (salvo needs to be set to 2 targets, etc) but that all share the dot, so without other handling, in some cases propagating a name unique to whatever implementation is looked up, like "explosive_shot_salvo", all the way to base actions would result in the wrong dot being looked up, when they all need to hook up to the same "explosive_shot". So volley and multishot both look up "explosive_shot_salvo" and then end up with an implementation of explosive shot with targets set to 2, but based on a action still named "explosive_shot" pulling up the "explosive_shot" dot it shares with every other implementation.

I think I always kinda glossed over it assuming it looked more like a map of name to action and was pulling up the same action in a case like that.

Jayezi added a commit that referenced this pull request Aug 13, 2024
Explosive Shot no longer explodes on ticks, but explodes once on expiry, with refreshes triggering an immediate explosion and leaving one tick for that expiry. BM casts with Explosive Venom carry their state from cast into the explosion, presumably the same would be the case for SV explosions from sources that have variable effectiveness.
Should clean up the get_background_action uses related to Explosive Shot mentioned in #9142.
Jayezi added a commit that referenced this pull request Aug 15, 2024
Clean up Serpent Sting action creation and other get_background_action usage as mentioned in #9142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants