quisquous / cactbot

FFXIV TypeScript Raiding Overlay
Apache License 2.0
793 stars 379 forks source link

RE: Tankbusters always sound #3531

Closed kaedys closed 2 years ago

kaedys commented 3 years ago

Summary

This is regarding https://github.com/quisquous/cactbot/pull/3432, and the change to remove the "CaresAbout" conditionals. This appears to be somewhat inconsistent, as some conditions (ex. Pillory in Holminster Switch) are still using an ad-hoc version.

However, the point is more around the "well, if users don't like it, they can disable it" conclusion. The issue, for me, is that I play multiple roles. I don't care about tankbusters as a DPS, but I very much do care about them when I'm tanking or healing. I'd like to propose either:

OR:

Personally, I prefer the second option, as it affords the greater amount of control and flexibility (ie. can set a tankbuster on one fight to show but not on another, depending on relevance to your role), but I'm not sure on the relative effort between the two, or which y'all feel would make more sense from a UX perspective.

gamescom15 commented 3 years ago

I also enjoyed the old CaresAbout behaviour. Having a toggle somewhere in the settings is preferable to just enabling them for everyone. (Mind you, the default can be to always show it as long as users have the ability to turn it off) Maybe something like:

On a related note, it is mentioned in #3432 that cactbot "already has way too many options". Maybe it's worth creating more Categories for e.g. Audio (Alarm sound volume, Alert sound volume ...) and Cactbot-specific strats (wormhole & e8s as off now, but that list might grow in the future)

quisquous commented 3 years ago

In Endwalker, feint and addle will mitigate both physical and magical damage. I think that means that all jobs have access to at least one piece of party mitigation that could apply to raidwides and tankbusters, and so "cares about" now applies to everybody. There are still alerts that don't show when they aren't related to your role (provokes, invulns, healing-specific mechanics), but raidwides and tankbusters apply to all roles.

If you aren't happy with this change in trigger behavior, my recommendation is that you should add your own user trigger that replaces the triggers that you want to change. You can add whatever new condition function there that you want for any triggers that you don't like, and hopefully there's only a small set of triggers that feel noisy to you but you still want some of the time. Somebody could even maintain a github repo with user overrides that do all of this that could be shared with people who want to preserve some behavior along these lines.

Re: cactbot-specific strats. In the future, those will also move to user triggers and that list will go away. I don't want to add a million options for all possible specific strats, and will instead link to user triggers that people can include in their user folder if they want to use different strats.

Re: Pillory having ad-hoc conditions. I'm taking care of those in #3534.

kaedys commented 3 years ago

I covered the user trigger thing. The problem is, you need to write a def for every single tankbuster, afaik. And since it replaces them, it isn't a good forwards-compatible solution, as you fail to get any other changes to that trigger in the future, and have to update your def for it for any breaking changes made to the trigger def.

Realistically, there just needs to be an easier way to set a notification as "I don't care about this". That's why I suggested the role-based checklists, it allows the users to define which ones they care about and don't, rather than it being set as a one-size-fits-all by the plugin, and only overrideable by creating an override def for each individual tankbuster or AoE in the game that you want to disable.

quisquous commented 3 years ago

I'm sorry to disagree with some more here. I know this isn't what either of you want to hear, but I believe that the current state of affairs (everybody sees tankbusters and raidwides) is just applying "cares about" to the changes in Endwalker. It also solves a constant confusion about why certain triggers appear in some cases but not in others.

Realistically, there just needs to be an easier way to set a notification as "I don't care about this"

I'm really not trying to be pedantic here, but there already exists config ui to set a notification as "I don't care about this". There's just not config ui to set a notification as "I don't care about this in these specific scenarios even when they apply to my role". I think if you don't want the one-size-fits all, there are several options available to you (user trigger files, forking cactbot, disabling triggers and writing your own in cactbot/trigg/something else) to make exactly the customization that you want to make.

The problem is, you need to write a def for every single tankbuster, afaik.

If you truly want to do this for every single tankbuster as you say in italics, please consider that you are asking me to do this same work that you are resisting, both on all existing content and into the future on new content. It's not even as simple as reverting the two changes I made; the manual role triggers would need to all be modified to handle these new options in a way that didn't cause redundant/duplicate output and would require extra care.

It seems unlikely to me that this is so critical that you need to update every single trigger in the game to behave this way. Maybe I am misunderstanding the situation, but it seems that it's more likely that there's ~10 triggers you care about in the current raid tier or in content that you are running a bunch that would make it feel less noisy and more comfy. This seems much more feasible to me.

And since it replaces them, it isn't a good forwards-compatible solution

You are not the first person who has said "overriding triggers is not forwards compatible", and I continue to disagree. Overriding entire triggers is more likely to keep working in the future than adding special case functions. I discussed this some in this pull request. But the short of it is that saying "I'm going to handle this whole trigger" is a looser coupling that it is more likely to keep working in the future. Content very very rarely changes.

The other option is to do what some people do and to fork cactbot into your own personal repo. You can update the files you want there, commit them, and periodically merge in new changes from cactbot. If you get conflicts, you'll see them and can then handle them. I would argue this is more pain then the above, but I'll mention it for completeness.

kaedys commented 3 years ago

Hmm, unfortunate. I still definitely disagree with you, both on the usefulness of such a feature, and on the relative scope of work between the potential solutions. It's also unfortunate that you still feel your particular opinion on which triggers people should care about is the only one that matters.

Realistically, it's far easier to implement a single solution that universally affects all triggers than it is to individually overwrite each trigger, both now and then again for each new set of content added going forward. The plugin already has the ability to customize the alert set for any given trigger. I fail to see how it would take extensive refactoring or run the risk of duplicate alert to simply add an additional setting, applied in the same area, which compares the user's current role against the set of roles they've set that trigger to display for, and if not set, suppress the alert entirely (as if they'd selected Disabled in the customization dropdown). You're already having to do logic in that area related to the user's selection in the dropdown.

In any case, your underestimate of the scope of the overrides is substantial. Perhaps I'm simply engaging with more content still than you do (I've only been playing this game for a few months, after all). Just in Shadowbringers, there's 13 dungeons, each of which contain 3 bosses, nearly all of which contain at least one tankbuster that I don't care to have an alert for (I nearly always use my Addle/group DR cooldowns on group damage rather than tankbusters). There's also 7 trials, and their extreme versions, 3 alliance raids (4-5 bosses each), and 12 levels of Eden. Overall, if I want to factor out the tankbuster in just Shadowbringers content (and that's ignoring all of the incidences of it that I run into during my plethora of roulettes that hit prior expansion content), that's at a minimum some 70 alert overrides. And there's certain to be a similar number in Endwalker, over the course of the content drops.

And for each of those overrides, it isolates me from any bugfixes, improvements, or other changes you make to those triggers going forward, unless I critically scan every commit for changes to those specific triggers and update mine for the same. That's especially relevant if I apply such override early in the content cycle (such as right when then Endwalker plugin updates are pushed out), as those are almost certain to have some tweaks and bugfixes applied.

On the flip side, adding in this feature would apply to every trigger currently in the game and every trigger added in the future, and wouldn't even be a specific change in each trigger definition. The concept I had would be for the changes to be applied at the same level as the settings are currently evaluated for the dropdown that selects the alert type (including, potentially, Disabled). Those settings would now include a mapping of which roles the users selected to show the alert (and again, the default for that could easily simply be all of them selected), and if the user's current role is not selected, the trigger simply acts as if Disabled had been set in the dropdown (with the exception that abilities targeted on the user will always display regardless of role).

In fact, such a system could easily replace the existing ad-hoc role conditions you have applied, simply by allowing triggers to specify an alternative set of default roles selected (for example, cleaved tankbusters, which you don't want displayed to DPS to avoid people inadvertently stacking for them, could be set by default as just healers and tanks, but could be turned on by DPS such as a raid leader that wants to know when they are happening).

Ultimately, it's your plugin. If you don't want to go that direction, well, that's simply unfortunate. But I think you're underestimating the benefit, and overestimating the changes required (and underestimating the work required for the "workaround" options). This is adding user customizability, the ability of the users rather than the plugin authors to determine which abilities they should care about and which they don't, which is almost never a bad thing to add.

quisquous commented 3 years ago

It's also unfortunate that you still feel your particular opinion on which triggers people should care about is the only one that matters.

This is a curious argument. I have suggested several different paths to implement the feature that you are asking for. I haven't ever said "I don't think this opinion matters". My perspective is more "I don't want to have to support this feature across all past and future triggers". cactbot is open source, and if you would like to implement this yourself, you can either add files in your user folder or fork cactbot and implement directly. I would be happy to give you pointers in either case.

As you say, this would be useful customization for users. For me, I just don't think this is what the bulk of users want and I don't think it's worth the effort to maintain it into the future for the small percentage of users who do want it. As with nearly everything, it's a tradeoff.

Perhaps I'm simply engaging with more content still than you do

It is unnecessary to enumerate content to make an argument. I am painfully aware of just how much content FFXIV has and exactly what cactbot does and doesn't support. That's why "maintain this feature across all triggers" has a cost.

But I think you're underestimating the benefit, and overestimating the changes required (and underestimating the work required for the "workaround" options).

You can't have it both ways. Either it's easy to implement this (in which case, what are you waiting for) or it's hard to implement this (in which case, why are you asking me to do so much work). The changes and the workaround are both roughly the same set of work.

kaedys commented 3 years ago

As with nearly everything, it's a tradeoff.

It's a fair point. And mine apologies if I made it sound like this is merely stubbornness on your part. As you say, I think this ultimately comes down to a judgment on how many users are likely to engage with the feature, versus how many are not.

The current options for customizing notifications are fairly basic, being confined to a handful of options as far as alert type, and then an option to set the actual text displayed. All other customization requires at least some knowledge of and comfort with editing code files. While the actual implementation of those custom triggers is likely something you (and I) find to be quite easy, from a mechanical perspective, I know many who would be overwhelmed by the mere prospect of trying to put together a custom trigger, regardless of how many help comments and examples were given, and regardless of how much of it was, tbh, essentially copy-paste with some tweaking. I think software engineers like you and I sometimes forget how daunting looking at code can be for those not familiar with coding as a concept.

That's why "maintain this feature across all triggers" has a cost.

But that was really my point. Handling this on a trigger-by-trigger basis is a huge amount of work, both now and for every single feature going forward. If, on the other hand, this were implemented in a single common function that all of the triggers leverage (or even a handful of them), like the same config set you store the existing options for displaying text, sound, and/or TTS, that the solution would be a set-it-and-forget-it without needed maintenance except where code that affects it actually changes. The default for triggers without a configured set of options could be all-roles-selected, and then maybe the triggers could override it if the trigger needs to be specific (but you'd only need to define it for triggers that are specific, and only if you want to go that direction; the existing role conditions are probably fine if you don't).

Now, unfortunately, I don't know Typescript (or Javascript, for that matter, my primary language is Go), and looking over the code for this plugin is in fact the first time I've encountered Typescript code, else I would have simply PR'd the changes by now. I have a vague idea of where the code for it might live and how it might work, but my knowledge of TS simply isn't up to the task ('yet', potentially).

The changes and the workaround are both roughly the same set of work.

Only if you presume the only possible solution on the plugin side is to edit every single trigger. And that's not what I'm proposing.

I'm proposing a new option created (I'd assume this would be in a single place, as I assume each trigger simply uses a template to generate its options section in the UI), and then the selection for those would be leveraged in a single or possible a handful of places, depending on how the triggers are actually handled (tbh, it's a bit difficult following the program flow, not knowing the particulars of TS syntax), where in some common function each of the triggers ultimately passes through before being displayed, it would check that optionset and simply return without doing anything if the player's role did not match the optionset. If the ability for triggers to set a default set of roles (thus obviating the need for role-based trigger conditions, though that's an optional enhancement, imo), that would be handled when generating the UI options, in much the same way that triggers can define the default alert text, duration, etc (which can then be overridden by the user).

Can you not see how there's a world of difference in the effort required between copying 70+ triggers, probably per expansion, versus implementing a single approach in a common function that automatically applies to all triggers merely by virtue of their existing templating?

quisquous commented 2 years ago

Coming back to old issues. I'm sorry for the disagreement here, but if this is something that you want I think your best bet is to fork cactbot and implement it yourself. I haven't heard anybody else ask for this or complain about the tankbusters in endwalker.