raidboss: Support for objects in trigger outputStrings #5551

valarnin · 2023-06-12T17:32:20Z

This is a preliminary PR to eventually allow more customization of outputStrings outputs by users. Eventually I'd like to see player names replaced with something like a player object with a default toString that returns player name, which could maybe be a helper on the PartyTracker object.

I have simulated that extremely basically with the data object test trigger added in test.ts. Users should be able to customize the outputStrings entry for text to print e.g. ${data.role}. I'm looking for feedback/thoughts on if this approach makes sense, or if this idea should be approached in a different way, before I fully flesh this out.

I'm not super happy with the logic in popup-text.ts, feels like it could have benefited with a helper func or something rather than switch/casing over type in two separate spots like that, or maybe a recursive function?

cactbotbot · 2023-06-12T17:33:04Z

@valarnin Thanks for your contribution! 🌵🚀

ui/raidboss/data/00-misc/test.ts

types/trigger.d.ts

test/helper/test_trigger.ts

ui/raidboss/popup-text.ts

valarnin · 2023-06-12T23:08:38Z

I think this is ready for general review now. I haven't tested the latest changes (ran out of time, I have another obligation in about 5 minutes), but the underlying code should be relatively complete.

I ended up moving the ShortName logic to Util for this rather than duplicate it in a third place, and cleaned up the duplicate code, hence the oopsy-related files being updated.

ui/raidboss/popup-text.ts

quisquous · 2023-06-13T01:39:50Z

resources/party.ts

+    if (!partyMember)
+      return;


This makes me a little worried in the face of the OverlayPlugin bug. I think this function should never return undefined. It should at least return what it can (name, shortName) and then elide other fields. And, ideally, if somebody asks for a property that doesn't exist, it should print a warning and pick ~some property rather than nothing. (Maybe shortName if that exists, and any key after that? Maybe these objects need a default: string field in case somebody or some code asks for a non-existent property and we need to fall back on something?)

Doing this would also solve the TODO in the raidboss test.ts where you don't get anything if you're not in a party.

I was thinking of PRing the adjustments to OverlayPlugin as my next project, a general cleanup of the way the PartyChanged event works under the hood.

As for the other points in this comment, agreed.

Also wanted to add that there are some times where triggers might call out to "stack on (non-party member)" for things like hunts, and so in this case even if OP was 100% working for party members, we'd still want to have reasonable fallback behavior.

One final question (and maybe there's no good answer here) is what to do if somebody enters ${data.name} (${data.role}) (e.g. #5554) but role is unknown. I guess it would just say name twice? Do we need a separate nameAndRole entry that could handle this? (Or maybe I am overthinking and this is overkill.)

I'm holding off on this piece until I can look at the OP issue in general, but I think the fallback logic is fuzzy and I'm not 100% sure what would be the best behavior here.

In general, what fallback should be used? Should job/role fall back to name or shortName, or just display ???? If we're displaying something like ???, should that be locale-'d e.g. Outputs.unknown, and if so, passing current locale to this location is complicated.

We could just fall back to shortName always for all objects (and then pick any field after that if it doesn't exist?), but in a general sense where objects are not always players, that seems a little weird.

One thought is that the object should put a (required?) default or fallback field that specifies what value to be used if a user or code specifies a sub param that doesn't exist. Personally, I think shortName is a reasonable choice to use for that value here. I think if there's always a valid fallback, then there's no need for ??? or locale plumbing.

Coming back to this PR, I still think that this is the missing piece here that prevents me from landing this PR. I feel like there needs to be some story around "what happens if a sub param is specified that doesn't exist". I think in general, it would be better to show something reasonable rather than ??? regardless of the OP party bug.

This is updated with the latest commit (also rebased to main). Hopefully this makes sense and lines up with what you're looking for?

Yes, although I also needed to do e9fcc96 in order for the "no prop specified" case to work.

resources/party.ts

types/trigger.d.ts

ui/raidboss/raidboss_options.ts

types/trigger.d.ts

Extract the ShortName logic/functions to Util, clean up duplicate code Narrow the typedef for params objects Add helpers to get party member param object Add config option for default display label for players Clean up handling of param objects

Co-authored-by: Adrienne Walker <enne@quisquo.us>

quisquous

Thank you!

quisquous · 2023-08-08T01:38:00Z

types/data.d.ts

@@ -42,6 +42,7 @@ export interface RaidbossData {
  CanCleanse: () => boolean;
  CanFeint: () => boolean;
  CanAddle: () => boolean;
+  partyMemberParam: (name: string) => PartyMemberParamObject | undefined;


Just to avoid polluting data even more, what do you think about making this be a function on PartyTracker since data.party is already a thing? I'd be fine with data.party.member(name) if you want to keep it brief.

I know this would require passing in nicknames and the default field as well, so maybe it's not ideal. Mostly curious if you think that's polluting party tracker with things it shouldn't know about or if that's cleaner.

I think the only way I'd be okay with this would be to pass the whole RaidbossOptions object in, rather than passing in individual values, and AFAIK that would cause issues with oopsy and jobs since their config options don't have that parameter?

oopsy has a nicknames field. It could arguably grow the default field and so oopsy could say people's roles rather than names. jobs doesn't really have a need for either. So, maybe it could take an optional object that has nick name and default that both RaidbossOptions and OopsyOptions is shaped like. (Or an object with optional nick name and default?)

My apologies if I derailed this PR with these comments. Mostly I just am trying to be cautious about adding unnecessary things to data since it's something shared with users. Happy to discuss more if you have time!

I'm guessing you're just very busy these days; please let me know if there's something I can do to help this along, as I'm still really excited about this change and being able to let users have more custom output for players.

Yeah, I've been very busy the last month or so, so I haven't had time to circle back around to this PR and figure out how to get things to pass through well.

I'm hoping to have time in the next week or two, but we'll see how work goes 🤷

Addressed this in c77169a

resources/party.ts

quisquous · 2023-08-08T01:48:48Z

resources/party.ts

+        id: '???',
+        job: 'NONE',
+        role: '???',


Given the fallbacks, do you think it would make sense to have all of these be optional other than the name/shortName. That way if somebody has a trigger that says ${player1.role} and something has gone awry, they'll get player1's name instead of ???.

We could do a priority as well, sure. Some sort of const array at the top and then loop through until we find a value should work?

I was saying something different; there's only two states, either we know everything or we only know name. I'm suggesting to make all the fields but name/shortName optional and possibly undefined so that in the case where we only know name, we only need to fill in name and shortName. That way if somebody specifies a missing field (that would normally be filled in but maybe it's the OP bug so it's not) then instead of getting '???' or 'NONE' when that's not useful, they would at least get the name. In other words, only fill in real values and never use dummy values or said differently never add something to this object that you wouldn't want displayed in a trigger.

Addressed this in 5121b99

ui/raidboss/data/00-misc/test.ts

valarnin · 2023-10-22T01:12:25Z

Closing.

This is #5551, with some additional changes on top to resolve a few conversations. Closes #5551. --------- Co-authored-by: valarnin <valarnin@gmail.com>

#5861) This is #5551, with some additional changes on top to resolve a few conversations. Closes #5551. --------- Co-authored-by: valarnin <valarnin@gmail.com> 5efd9ee

github-actions bot added raidboss test style labels Jun 12, 2023

valarnin changed the title ~~raidboss: Preliminary support for objects in trigger outputStrings~~ raidboss: Support for objects in trigger outputStrings Jun 12, 2023

quisquous reviewed Jun 12, 2023

View reviewed changes

ui/raidboss/data/00-misc/test.ts Show resolved Hide resolved

types/trigger.d.ts Outdated Show resolved Hide resolved

test/helper/test_trigger.ts Outdated Show resolved Hide resolved

quisquous reviewed Jun 12, 2023

View reviewed changes

ui/raidboss/popup-text.ts Show resolved Hide resolved

ui/raidboss/popup-text.ts Outdated Show resolved Hide resolved

ui/raidboss/popup-text.ts Show resolved Hide resolved

github-actions bot added resources config oopsy labels Jun 12, 2023

valarnin marked this pull request as ready for review June 12, 2023 23:06

quisquous reviewed Jun 13, 2023

View reviewed changes

types/trigger.d.ts Show resolved Hide resolved

quisquous mentioned this pull request Jun 14, 2023

raidboss: add role callout to p11s dark & light tethers #5554

Merged

quisquous mentioned this pull request Aug 3, 2023

raidboss: Replace player names with job names in popup text #5745

Closed

valarnin and others added 9 commits August 7, 2023 19:47

Preliminary support for objects in trigger outputStrings

2a6c8c9

Limit param keys to only top-level props

314ec15

Extract the ShortName logic/functions to Util, clean up duplicate code Narrow the typedef for params objects Add helpers to get party member param object Add config option for default display label for players Clean up handling of param objects

Fix missing changes, thanks WSL

13f5c5f

Apply suggestions from code review

9b93f2f

Co-authored-by: Adrienne Walker <enne@quisquo.us>

Cleanup per review, fix issue with suggestion

6ad450f

Rebase to upstream/main

1a299af

Default paramObjectFromName return object to something sane

917e08d

Fix test errors

84b8d34

Fix other test/lint errors

f289df5

quisquous reviewed Aug 8, 2023

View reviewed changes

Change type on role

2ad88a5

quisquous mentioned this pull request Oct 21, 2023

raidboss: Support for objects in trigger outputStrings #5861

Merged

valarnin closed this Oct 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raidboss: Support for objects in trigger outputStrings #5551

raidboss: Support for objects in trigger outputStrings #5551

valarnin commented Jun 12, 2023

cactbotbot commented Jun 12, 2023 •

edited

Loading

valarnin commented Jun 12, 2023

quisquous Jun 13, 2023

valarnin Jun 13, 2023

quisquous Jun 13, 2023

quisquous Jun 13, 2023

valarnin Jun 13, 2023

valarnin Jun 13, 2023

quisquous Jun 13, 2023 •

edited

Loading

quisquous Aug 3, 2023

valarnin Aug 8, 2023

quisquous Oct 21, 2023

quisquous left a comment

quisquous Aug 8, 2023

valarnin Aug 8, 2023

quisquous Aug 8, 2023

quisquous Sep 10, 2023

valarnin Sep 10, 2023

quisquous Oct 21, 2023

quisquous Aug 8, 2023

valarnin Aug 8, 2023

quisquous Aug 8, 2023 •

edited

Loading

quisquous Oct 21, 2023

valarnin commented Oct 22, 2023

raidboss: Support for objects in trigger outputStrings #5551

raidboss: Support for objects in trigger outputStrings #5551

Conversation

valarnin commented Jun 12, 2023

cactbotbot commented Jun 12, 2023 • edited Loading

valarnin commented Jun 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quisquous Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quisquous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quisquous Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valarnin commented Oct 22, 2023

cactbotbot commented Jun 12, 2023 •

edited

Loading

quisquous Jun 13, 2023 •

edited

Loading

quisquous Aug 8, 2023 •

edited

Loading