[#1405] Add TraitFlow & application methods

[arbron]

Add TraitFlow and the application methods to TraitAdvancement.

[Fyorl]

I had some trouble following the logic in TraitAdvancement#unulfilledChoices and TraitAdvancement#availableChoices but I was able to get to grips with it in the end. I have attached a diff of a small refactoring around those methods that I think does a reasonable job of making it a bit clearer (to me at least) of what we are doing in them. Please let me know what you think, or if I have missed anything. The diff contains a few changes to things that I already flagged in this PR but I've left them flagged in case we do not want to incorporate any or all of the diff.

I think if we were to do a concerted pass over the language used in the comments and the naming of variables/classes, then we might be able to clear things up and make some of the inner-workings a bit more understandable. We should try to be consistent with where we use the word 'grant', and what it means, and how it contrasts with a 'choice', for example. We can consider if SelectChoice is really the best name for what this class is. Naming is hard though, even I struggled with it in my attached refactor, so let's just try to do our best here.

Another thing that might help reduce complexity in both the config UI and our code is to consider if we really need the 'exclusive' choice mode. I couldn't find any examples of where this would be used when doing my testing, at least in official content. Each option within the choice 'groups' are already exclusive, so it seems that outer inclusivity, with inner exclusivity covers all the cases that I could see. We should not have to support every imagined possibility if we do not have to, especially if it helps us reduce complexity and maintenance burden.

Finally, it would be nice if we could improve the display of the 'grants' somewhat. If we look at the auto-generated hint for a test advancement I put together that includes the Druid's list of weapon proficiencies, it looks quite messy. Additionally, each individual weapon proficiency takes up a slot which is not really ideal in my opinion.

It's possible to override the hint of course, but due to limitations with sanitisation, this can't be an HTMLField, so the user can't input something more nicely-formatted like so:

If we were to automatically generate something like the above, where the grants are grouped into categories and do not appear in the slots, then we can side-step this limitation. Note that this would apply just to grants, for choices I think the auto-generated text is sufficient.

There are some complications where grants are concerned in the allowReplacements case, but I don't think they are insurmountable.

unfulfilled-simplify.patch

[arbron]

Another thing that might help reduce complexity in both the config UI and our code is to consider if we really need the 'exclusive' choice mode. I couldn't find any examples of where this would be used when doing my testing, at least in official content. Each option within the choice 'groups' are already exclusive, so it seems that outer inclusivity, with inner exclusivity covers all the cases that I could see. We should not have to support every imagined possibility if we do not have to, especially if it helps us reduce complexity and maintenance burden.

Exclusive mode was made to handle something like this:

This is admittedly a rare case, but I have seen it pop up here and there.

Finally, it would be nice if we could improve the display of the 'grants' somewhat. If we look at the auto-generated hint for a test advancement I put together that includes the Druid's list of weapon proficiencies, it looks quite messy. Additionally, each individual weapon proficiency takes up a slot which is not really ideal in my opinion.

They do take up a lot of space, but the intended will rarely have that many. Specifically its not generally encouraged to handle multiple trait types with a single advancement. I have to support it for expertise purposes, but a more common use would look like how I have druid set up in the SRD branch #2544:

Separating them like this is necessary to be able to set up multiclassing properly, and allows you to take advantage of the trait-specific automatic title and icon (all of those names on Druid are the default).

The flow view is still pretty long, but not unreasonably so, and this is basically the longest it will ever get:

Maybe we could adjust the spacing a bit and some additional styling to make it look better, but I didn't want to change the appearance of the grants to be too separate from the choices so it will be clear what you are getting when you hit next.

Also, the class journal page uses separate advancements to generate a list of class proficiencies:

[Fyorl]

Exclusive mode was made to handle something like this:

This is admittedly a rare case, but I have seen it pop up here and there.

Do you know where this is from? Because it seems like a mistake: The rules for customising a background in the PHB are "choose a total of two tool proficiencies or languages", but the text you referenced is asking the player to choose between 2 languages or a single tool proficiency.

[Fyorl]

The flow view is still pretty long, but not unreasonably so, and this is basically the longest it will ever get:

It seems like they're moving away from these long lists of individual weapon proficiencies in the playtest anyway, so I guess it doesn't make much sense to add special code for these rare cases.

[Zanderaf]

It looks like something from maybe the OneD&D playtests? At least appearance wise, though I cant find anything that matches it.

[arbron]

Do you know where this is from? Because it seems like a mistake: The rules for customising a background in the PHB are "choose a total of two tool proficiencies or languages", but the text you referenced is asking the player to choose between 2 languages or a single tool proficiency.

That is from the Black Flag alpha docs. I believe I have seen this pattern used in other 3rd party content, but finding which of my sourcebooks has it is a big task :).

[Fyorl]

That is from the Black Flag alpha docs. I believe I have seen this pattern used in other 3rd party content, but finding which of my sourcebooks has it is a big task :).

It seems difficult to justify all the added complexity across the entire trait advancement pipeline (including configuration UI and implementation) in order to support something that does not seem to really be part of the core system design.

If we look at the table on origin customisation in Tasha's, they are all 1:1 trades, there's not really any notion of '2 skills are worth one weapon proficiency' or something like that, and I believe that's very much intentional.

Of course homebrew or 3rd party content can write what it wants, but I think in those cases I'd really prefer to push the complexity onto them rather than have it in the system because it's so niche.

Additionally, if we remove 'exclusive' mode and all choices are inclusive, then it seems like we don't even need to track multiple choices at all, they could simply be split into separate advancements, which seems to be the recommendation anyway. This would represent a significant decrease in complexity of the data model, and the configuration UI. I think any concerns about intuitiveness of the UI would evaporate.

I feel quite bad to be bringing this up now, when it should have been feedback right from conception of the data model, but I think I was trusting that the underlying trait landscape was proportionally complex. Undergoing the exercise of testing this and perusing places where traits are granted in official content, the scope seems to be limited to: Grants (and replacements), Choose X, Expertise, and Upgrade, which are all things we have built support for here, but these 'meta choice groups' did not really appear from what I could tell.

Do you have any examples of backgrounds or class features that we would need this extra logic to accommodate? Otherwise I think we could pare back the implementation considerably.

[arbron]

It seems difficult to justify all the added complexity across the entire trait advancement pipeline (including configuration UI and implementation) in order to support something that does not seem to really be part of the core system design.

I can take out exclusive choice mode.

Do you have any examples of backgrounds or class features that we would need this extra logic to accommodate? Otherwise I think we could pare back the implementation considerably.

Multiple choices will need to remain in because there is official content that requires it, for example the Haunted One background from Curse of Strahd:

[Fyorl]

Multiple choices will need to remain in because there is official content that requires it, for example the Haunted One background from Curse of Strahd:

It's a shame that such awkward exceptions exist, but that's the sort of example I was looking for, thank you. I don't have any objections to keeping the inclusive choice groups then in that case.

[arbron]

@Fyorl Stripped out exclusive mode (and fixed two bugs).

[Fyorl]

The simplification looks good, and good job catching those bugs. One very minor issue with a comment that I flagged, and one other question, otherwise feel free to merge away.

[Fyorl]

Can't this be done in AdvancementManager.flowsForLevel? Or are there other paths to instantiating flows?

[arbron]

Because that is a static method we don't have access to the AdvancementManager instance in that location, so adding it would require modifying every place we call it. I figured this was a much more manageable change.

[Fyorl]

Yeah that's fair enough, it's not a big deal either way