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

Add basic support for physical implement items for Thaumaturge #16759

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pedrogrullada
Copy link
Collaborator

Either grant a generic implement item, or tag an existing item as an implement. No automation yet, but the plan is to add suboptions to denote which implement is being held, and leverage that for automation from there.

Tagging @mysurvive, your input would be appreciated here! Trying to take some stuff off your plate.

@pedrogrullada pedrogrullada added the pr: data update Updates to existing actors and items label Oct 4, 2024
@TikaelSol TikaelSol marked this pull request as draft October 4, 2024 21:00
@mysurvive
Copy link
Contributor

I finally had a minute to pull this and look at it. I am a fan. I think it's a good start to getting implements working properly. There are some other things I would consider to make it really great (in no particular order, and some should be in a different PR(s) to not conflate this one):

  1. Support homebrew implements - Currently, if a homebrew implement is selected, there is no choiceset for choosing/creating the physical item from the inventory.
  2. Create some way to get the selected physical implements easily. This could be an actor flag or a new getter on the actor (the former is easier to implement and would require less maintenance). Adding something like selected-implement or similar might be able to produce a workaround way of doing this so you could do something like _token.actor.itemTypes.equipment.map(i => i.system.traits.otherTags.includes("selected-implement") ? i : null).filter(i => i != null) to get the selected implements.
  3. A way to easily change physical implements: "If you acquire a new object of the same general implement type, you can switch your implement to the new object by spending 1 day of downtime with the new item." (Dark Archives p. 32 - First Implement and Esoterica). I don't have a recommendation for a good fix for this... it would probably require some new tech to force choicesets to allow re-selecting their choices.
  4. A better indicator for what is a physical implement. I saw that the selected implements get the unique rarity, but I'm not necessarily sold that this is enough. Devs permitting, I would suggest a new trait: "Implement" or something similar, with an indicator on the sheet (not trying to toot my own horn, but I am a huge fan of pf2e exploit vulnerability having an icon on the sheet to easily be able to see at a glance what you have selected as implements).

From a data entry perspective, I think this is so close to being ready. 1 & 2, I think, would be the most pressing tasks to tackle. Number 3 would require a new PR with some new tech, but I think users would get frustrated without the ability to change implements, but I think it doesn't affect the Data Entry PR at all. Number 4 is purely quality of life and can be kicked down the road, I think.

Someone can correct me on the analysis as well, this is just my 2 cents.

@pedrogrullada
Copy link
Collaborator Author

pedrogrullada commented Oct 5, 2024

Thank you for taking the time to look at it!

  1. Support homebrew implements - Currently, if a homebrew implement is selected, there is no choiceset for choosing/creating the physical item from the inventory.

Since the physical implement is granted by that specific implement feature (i.e. The Lantern feature grants the Lantern Implement), all a 3PP would have to do is provide their own physical implement to be granted with their feature. This means it would be on the 3PP to add the corresponding Choice Sets, which should be relatively straightforward from copying another implement's rule elements.

  1. Create some way to get the selected physical implements easily. This could be an actor flag or a new getter on the actor (the former is easier to implement and would require less maintenance). Adding something like selected-implement or similar might be able to produce a workaround way of doing this so you could do something like _token.actor.itemTypes.equipment.map(i => i.system.traits.otherTags.includes("selected-implement") ? i : null).filter(i => i != null) to get the selected implements.

A physical implement has a tag of the form implement:slug (like implement:lantern or implement-mirror) when selected as such (or granted). I'm open to adding a different one like selected-implement that they all share if that would make it easier though. It's pretty harmless.

  1. A way to easily change physical implements: "If you acquire a new object of the same general implement type, you can switch your implement to the new object by spending 1 day of downtime with the new item." (Dark Archives p. 32 - First Implement and Esoterica). I don't have a recommendation for a good fix for this... it would probably require some new tech to force choicesets to allow re-selecting their choices.

Yeah, there isn't a bespoke way to do that right now, might fall under the umbrella of a more general feature request in regards to retraining. For now, doing the level-0-and-back dance will allow you to pick new implements or assign existing items implement status. There is the drawback that generic physical items granted before won't be deleted, but Inventor faces that same limitation at the moment.

  1. A better indicator for what is a physical implement. I saw that the selected implements get the unique rarity, but I'm not necessarily sold that this is enough. Devs permitting, I would suggest a new trait: "Implement" or something similar, with an indicator on the sheet (not trying to toot my own horn, but I am a huge fan of pf2e exploit vulnerability having an icon on the sheet to easily be able to see at a glance what you have selected as implements).

I had a similar thought. From a data entry perspective, a description Item Alteration could add a little blurb indicating that the item is an implement. It's not available at a glance the same way an icon is, but it can be a decent stopgap.
image
image

Let me know what you think. I could add these description addenda to this PR, as well as alter the physical implement tags to be more identifiable, or add a new one shared among them all you could use.

@mysurvive
Copy link
Contributor

Since the physical implement is granted by that specific implement feature (i.e. The Lantern feature grants the Lantern Implement), all a 3PP would have to do is provide their own physical implement to be granted with their feature. This means it would be on the 3PP to add the corresponding Choice Sets, which should be relatively straightforward from copying another implement's rule elements.

This is fair and valid, I think. It will just need documentation and visibility.

A physical implement has a tag of the form implement:slug (like implement:lantern or implement-mirror) when selected as such (or granted). I'm open to adding a different one like selected-implement that they all share if that would make it easier though. It's pretty harmless.

I think that implement:<implement> or implement:<rank>:<implement> should be a roll option added to the item rather than being an otherTag and the otherTag should be selected-implement. Trent went so far in the EV module to make an implement:<numeric rank>:<implement> roll option also, because you can do math on some of the implement effects based on the numeric rank of the implement. otherTags being more generic is more useful, in my opinion and will make it easier to parse actor items, whereas the specific implement having a roll option would allow predication off of that. The roll option is currently how the EV module currently applies implement effects. I am not sure whether having the name of the implement itself in otherTags is particularly useful. If you want to futureproof it fully in case you think otherTags might need to know what implement it's referring to, both can be added.

Yeah, there isn't a bespoke way to do that right now, might fall under the umbrella of a more general feature request in regards to retraining. For now, doing the level-0-and-back dance will allow you to pick new implements or assign existing items implement status. There is the drawback that generic physical items granted before won't be deleted, but Inventor faces that same limitation at the moment.

Agreed. 100%.

I had a similar thought. From a data entry perspective, a description Item Alteration could add a little blurb indicating that the item is an implement. It's not available at a glance the same way an icon is, but it can be a decent stopgap.

I think this is a good stopgap. Looks really clean too.

@pedrogrullada
Copy link
Collaborator Author

pedrogrullada commented Oct 6, 2024

I think that implement: or implement:: should be a roll option added to the item rather than being an otherTag and the otherTag should be selected-implement. Trent went so far in the EV module to make an implement:: roll option also, because you can do math on some of the implement effects based on the numeric rank of the implement. otherTags being more generic is more useful, in my opinion and will make it easier to parse actor items, whereas the specific implement having a roll option would allow predication off of that. The roll option is currently how the EV module currently applies implement effects. I am not sure whether having the name of the implement itself in otherTags is particularly useful. If you want to futureproof it fully in case you think otherTags might need to know what implement it's referring to, both can be added.

Roll options on the items are tricky, there is no great way to do it with just data entry tools. Other tags is the best way we have to get that kind of context into specific items.

However, the rank of the implement could be made available via roll options set by Implement Adept et. al. when picking which implement you're upgrading (it currently only sets a flag on the item itself, but adding the roll option is trivial, and the flag could be made into an actor flag if need be). If doing math over numerical implement ranks is necessary, we could store them as actor flags too. Would those two things be sufficient for what you need? At the very least, I'm fairly certain the roll options are something we will need anyway.

As far as predication is concerned, I'm envisioning mergeable suboptions as the way to actually manage implements, with the physical implements being more of an inventory tracking tool.

image

@mysurvive
Copy link
Contributor

The only reason that I suggested roll options on the item itself is to automate handedness on the item, which would eliminate the need for what you show in the picture. The roll option would just work:tm: when held instead of having to flip through toggleable/selectable roll options. I haven't messed with data entry tools in a while, so I wasn't sure whether the value field could derive its value from a flag, or something similar. If that is a limitation, then it's a limitation!

There are some cases where the implement numeric rank is useful. Here's how we set the flag for the lantern light radius
{ key: "ActiveEffectLike", mode: "upgrade", path: "flags.pf2e-thaum-vuln.lantern.radius", value: "@actor.attributes.implements.lantern.rank*10+10", phase: "afterDerived", }

And then the TokenLight effect itself gets this:

{ key: "TokenLight", label: "Lantern Implement Light", predicate: ["lantern-implement-lit"], value: { alpha: 0.45, animation: { intensity: 1, speed: 2, type: "flame", }, attenuation: 0.4, bright: "@actor.flags.pf2e-thaum-vuln.lantern.radius", color: "#ffae3d", dim: "@actor.flags.pf2e-thaum-vuln.lantern.radius*2", shadows: 0.2, }, }

Of course, as you can see, we do some wrapping of the derived actor data, but the principle stays the same.

@pedrogrullada
Copy link
Collaborator Author

pedrogrullada commented Oct 7, 2024

The only reason that I suggested roll options on the item itself is to automate handedness on the item, which would eliminate the need for what you show in the picture. The roll option would just work:tm: when held instead of having to flip through toggleable/selectable roll options. I haven't messed with data entry tools in a while, so I wasn't sure whether the value field could derive its value from a flag, or something similar. If that is a limitation, then it's a limitation!

The issue is that we can add roll options to generic implement items in the compendium, but not to any arbitrary item the thaumaturge decides to make their implement. Of course, that's an issue if inventory handedness is the way we want to track these, which I'm not super on board with, but I'll ask about it. If that's the route we were to go on, that would need some dev work. I'm personally more of a fan of the suboptions.

Thank you for the rank number examples. It doesn't seem necessary in that case, as a second predicated Token Light rule element could accomplish it too, but I can see how that could make it cleaner. I think that'll be something to consider once we get into automating this stuff!

EDIT: We've decided to drop the support for picking arbitrary items as implements for now (except weapon), that way we will be able to have control over the rules on the item, at least until there is a solution for picking other items as implements. That means suboptions will not be needed either.

@pedrogrullada
Copy link
Collaborator Author

Alright, some updates. For weapon implement, we track it via its Grant Item resulting in the weapon-implement:equipped roll option. For non-weapon physical implements, we add a roll option of the form <implement>-implement:equipped which will be active when they're equipped. Like this, the actor's roll options have the context of which implements are being held (note the unequipped Lantern implement):

image

Each implement also has the implement-held roll option, for any feature that requires an implement to be held at all, regardless of which it is. We could add a counter to these roll options, but I haven't seen a use for that yet, aside from perhaps turning off some automation if more than two implements are held, which seems a bit unnecessary.

This does leave the ability to make specific items into implements as a future request, which would require some way to keep track of those chosen items as roll options other rule elements could predicate off of, but this seems like a good base to start automating stuff.

@nikolaj-a
Copy link
Collaborator

Consider implement:<implement>:equipped instead of <implement>-implement:equipped.

@pedrogrullada
Copy link
Collaborator Author

Consider implement:<implement>:equipped instead of <implement>-implement:equipped.

The weapon implement roll option is created through tracking the Grant Item, which sluggifies the flag and turns it into the prefix for the weapon roll options, resulting in weapon-implement:equipped, but that method can't yield suboptions like that. I chose the other roll options to match that for the sake of consistency.

If there is a way we can have roll options on the chosen weapon of the form implement:<implement>:equipped that would certainly be a bit nicer, but I'm not sure there's a way.

@xyzzy42
Copy link
Contributor

xyzzy42 commented Oct 14, 2024

What's the flow like for adding a Weapon implement?

Seems like it would be:
Add class to character
...which automatically adds First Implement feature, which has ChoiceSet for type, choose Weapon.
...which Grants Weapon feature, which has ChoiceSet for physical item.
...shows long list with every level 0 one-handed weapon, adds to character sheet. Doesn't deduct cost.

Lets say we want to change the weapon used. Do this:
Level character to 0.
Level to 1.
Choose Weapon again.
Choose new weapon type from list.
Copy all runes, materials, or any other rules, flavor text, etc. from old weapon to new weapon.
Delete old weapon still on character sheet.
Re-level the remaining levels.

Any time we need to re-level the character, e.g. to choose a different implement or just to fix issues, it's a very common way to solve problems, we'll have to re-create the implement weapon by copying all the data to new weapon, since it always adds a new item.

@xyzzy42
Copy link
Contributor

xyzzy42 commented Oct 14, 2024

The weapon implement roll option is created through tracking the Grant Item, which sluggifies the flag and turns it into the prefix for the weapon roll options, resulting in weapon-implement:equipped, but that method can't yield suboptions like that. I chose the other roll options to match that for the sake of consistency.

Could the weapon implement feature have a RollOption implement:weapon:equipped that predicates on weapon-implement:equipped? That way it has the implement: form too, and the rest can use it and still match.

I also noticed that ALL the item options for the weapon implement show up in all rolls. E.g., here's a saving throw:
image

@xyzzy42
Copy link
Contributor

xyzzy42 commented Oct 14, 2024

This does leave the ability to make specific items into implements as a future request, which would require some way to keep track of those chosen items as roll options other rule elements could predicate off of, but this seems like a good base to start automating stuff.

The Thaumaturge module has this ability already. While it's pretty much mandatory to pick a specific item for a weapon, it also common for other items. I.e., as soon as the player acquires a wand, they'll make that the wand implement, since they might as well be holding a wand that can cast a spell vs one that can't.

@pedrogrullada
Copy link
Collaborator Author

Could the weapon implement feature have a RollOption implement:weapon:equipped that predicates on weapon-implement:equipped? That way it has the implement: form too, and the rest can use it and still match.

Yeah, that would be a way to do it.

I also noticed that ALL the item options for the weapon implement show up in all rolls. E.g., here's a saving throw:

That's what track does. It dumps all the roll options. We only need the :equipped one, but since we can't control the rule elements on the weapon, that's the only way for other rule elements to have it available, as far as I can tell.

@xyzzy42
Copy link
Contributor

xyzzy42 commented Oct 14, 2024

since we can't control the rule elements on the weapon, that's the only way for other rule elements to have it available, as far as I can tell.

That's effectively what the thaum module does. Controls the REs on the weapon or other physical implement.

Perhaps an ItemAlteration that allows adding REs would be a good way to do that?

@pedrogrullada
Copy link
Collaborator Author

Perhaps an ItemAlteration that allows adding REs would be a good way to do that?

Even just adding roll options to the item would be enough too. Of course, adding REs accomplishes that and more, but I've no idea how stable something like that would be.

@xyzzy42
Copy link
Contributor

xyzzy42 commented Oct 14, 2024

Even just adding roll options to the item would be enough too.

The existing automation in the thaum module for this, "When you use Implement's Interruption and fail (but don't critically fail) the Strike, you deal 1 damage of the weapon's normal type," uses an RE on the weapon itself to add the damage of the correct type as a note to failed strikes with the weapon. If the Note RE was instead of the class feature, it wouldn't be able to reference {item|} data from the weapon.

All the other rules look like they don't need to be on the physical item itself. Of course all the REs need to gain a predicate to only be in effect when the physical item is equipped. Being on the physical item does that automatically.

@pedrogrullada
Copy link
Collaborator Author

pedrogrullada commented Oct 16, 2024

Even just adding roll options to the item would be enough too.

The existing automation in the thaum module for this, "When you use Implement's Interruption and fail (but don't critically fail) the Strike, you deal 1 damage of the weapon's normal type," uses an RE on the weapon itself to add the damage of the correct type as a note to failed strikes with the weapon. If the Note RE was instead of the class feature, it wouldn't be able to reference {item|} data from the weapon.

We can probably live without automating that bit on our end (or do so at a lesser extent). If we do get the ability to mark items via rule element such that other rule elements can tell if they're equipped (however that's done), at that point we can also make arbitrary implement choice work too. Until then, we're stuck going generic with all but weapon (or take a different approach entirely, such as the mergeable suboptions I toyed with earlier)

EDIT: Actually, now that I think about it, it shouldn't be necessary for the Implement's Interruption adept benefit. We can do the same as the Swashbuckler's Precise Strike failure note.

@xyzzy42
Copy link
Contributor

xyzzy42 commented Oct 16, 2024

Have you thought about Dedication support? It makes a distinction between choosing an implement, when taking the dedication feat, and having support for the implement's abilities, when taking the Implement Initiate archetype feat. There are two existing PRs out to allow choosing implements for the dedication, #16265 and #14678

@pedrogrullada
Copy link
Collaborator Author

Have you thought about Dedication support? It makes a distinction between choosing an implement, when taking the dedication feat, and having support for the implement's abilities, when taking the Implement Initiate archetype feat. There are two existing PRs out to allow choosing implements for the dedication, #16265 and #14678

I have, though it's been a while since I looked at those specific PRs, but this shouldn't be incompatible with them. You receive the implement whether you're main class or not. The rule elements in the features will need to be appropriately predicated once we start actually adding automation, of course.

@xyzzy42
Copy link
Contributor

xyzzy42 commented Oct 16, 2024

It does interact, because #16265 chooses the implement in the dedication, but does not grant the class feature for the specific implement. That's granted when Implement Initiate is chosen, since none of the benefits tied to the implement are gained until that time.

But by putting the rules to add the physical items on the implement class feature, it will be necessary to add the class feature at the same time the dedication is taken. Which means any rules added by the class feature will apply and grant the benefits of the implement, which is wrong.

@pedrogrullada
Copy link
Collaborator Author

pedrogrullada commented Oct 16, 2024

Which means any rules added by the class feature will apply and grant the benefits of the implement, which is wrong.

The rule elements will just need to be appropriately predicated, same way Champion causes do, for example. It's something I could've added here while I was at it, but isn't relevant yet, since Thaumaturge Dedication in the main branch doesn't interact with the implement features (yet).

I didn't see that your PR grants the Implement feature with Implement Initiate, and this does mess with it. I would probably go the champ route instead.

@xyzzy42
Copy link
Contributor

xyzzy42 commented Oct 16, 2024

The rule elements will just need to be appropriately predicated, same way Champion causes do, for example

So that was the approach taken in #14678, but it doesn't work. The problem is GrantItem REs don't re-evaluate on adding feats. So if you predicate the GrantItem for, e.g., Implements Interruption action based on having Implement Initiate feat, it doesn't grant the action when the feat it added, because the grant item doesn't re-evaluate.

It's solved in the other PR, and I have already implemented full dedication support, it's just waiting for action on the PR.

I didn't see that your PR grants the Implement feature with Implement Initiate.

I include a screen shot that shows it, so I guess that tells me no one has even bothered to look at it.

@pedrogrullada
Copy link
Collaborator Author

pedrogrullada commented Oct 16, 2024

So that was the approach taken in #14678, but it doesn't work. The problem is GrantItem REs don't re-evaluate on adding feats. So if you predicate the GrantItem for, e.g., Implements Interruption action based on having Implement Initiate feat, it doesn't grant the action when the feat it added, because the grant item doesn't re-evaluate.

It's solved in the other PR, and I have already implemented full dedication support, it's just waiting for action on the PR.

It just needs an update, which is admittedly awkward, but does work. I'd rather see if we can get a solution to make that workflow better, like an update when a feat is added before the Grant is reevaluated or something, than trying to get around it.

I include a screen shot that shows it, so I guess that tells me no one has even bothered to look at it.

It just tells you that I hadn't looked at it or didn't remember it. I was just speaking on my own behalf. Let's not get sidetracked, please.

@xyzzy42
Copy link
Contributor

xyzzy42 commented Oct 17, 2024

I still see a few major regressions over what players have now with the module.

  1. Every time a player re-levels the character the weapon implement becomes a mundane weapon and a new item is created as a weapon implement. It's necessary to copy all changes: runes, materials, custom flavor text, item name, icon change, extra REs for other abilities, and so on, from the old item to the new item. Re-leveling is something done with some frequency to make new choices for class features or as a often suggested problem solving step to update an actor with some sort of out of date data on it.
  2. There is no way to select an existing items for implements. The RAW have only an open-ended "object of the same general implement type" requirement, so any implement could possibly be some sort of existing item. Which is quite often done, if it's necessary to use up a hand to hold the implement anyway, might as well hold an item that has some extra ability. And of course a weapon implement thaumaturge will want to change items every time they acquire an upgraded weapon, which is not possible.
  3. Which items are implements isn't as clearly indicated on the character sheet.

Maybe it's not possible to re-implement this in a way that is near parity with the existing implementation using only existing rule elements? Perhaps the approach taken in the module should be used and some way to mark existing items needs to be created in the system to adequately handle the feature.

@pedrogrullada
Copy link
Collaborator Author

Agreed, as I said before, once we can keep track of the carry type of specific items in some way, we get the ability to choose implements from inventory, which takes care of the weapon issue too.

As for a clearer indicator on the sheet than description addenda via Item Alteration for which items are implements, I'm not certain what the best approach would be (I'm aware the module uses an icon). Perhaps a name Item Alteration? Definitely lower priority than the first part though.

I personally would prefer if we could have the ability to choose implements from the inventory right out of the gate. That will mean making a feature request for the prerequisite features, but at least we have a good idea of what they are. That might take a while with how busy the devs are, but this is only a draft for a reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: data update Updates to existing actors and items
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants