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

Update Thaumaturge Features #14678

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mysurvive
Copy link
Contributor

This adds the thaumaturge-implement-initiate otherTag.

While not inherently or necessarily important for the Thaumaturge class, it allows implement features that have grantItem features the ability to predicate on the tag - this means that the Thaumaturge Dedication doesn't grant these actions unless they also take the Implement Initiate feat.

The Thaumaturge Dedication is also given a choiceSet to choose an implement, and the thaumaturge-implement-initiate otherTag is granted only when the Implement Initiate feat is granted.

@TikaelSol TikaelSol added the pr: data update Updates to existing actors and items label May 11, 2024
@mysurvive mysurvive force-pushed the Update_Thaumaturge_Features branch from 549d567 to 14d09d7 Compare May 11, 2024 14:44
@TikaelSol
Copy link
Collaborator

This one is going to take a bit longer to review.

@xyzzy42
Copy link
Contributor

xyzzy42 commented May 14, 2024

This seems reasonable to me. The Thaumaturge class features already have an implement selection choiceset and grantitem. The archetype also grants an implement, so it seems like it should have the choiceset and grantitem implement too.

But the archetype doesn't give the same things at first, so the flag and predicate are needed to avoid giving actions until the archetype Implement Initiate feat is taken.

The only thing I thought of was the tag could be reversed, so it's thaumaturge-implement-archetype. All the predicates would be inverted from what they are now. This way the ItemAlteration on the first/second/third implement features could go away, since no tag is the default case and it only needs to be added for archetype + not feat:implement-initiate.

@TikaelSol
Copy link
Collaborator

So this looks good but we don't have many v11 releases left and don't want to have to chase down anything this may cause issues with going forward, so we'll delay merging until after the last v11 release next week. Then we can get it in and ready for the non-beta v12 release where we could fix anything that needs fixing.

@xyzzy42
Copy link
Contributor

xyzzy42 commented May 14, 2024

So there wouldn't be a v11 release with this change? I guess if the Thaum Vuln module wants to become v12-only and depend on this change or remain v11 compatible and would around it would be something to consider.

@mysurvive
Copy link
Contributor Author

mysurvive commented May 15, 2024

I think it's fine and makes sense to push this to v12, especially since existing thaums/diet thaums will need to level dance/refresh feats to apply the new rules. I'll also let Tik decide whether it makes more sense to merge this as-is or changing to Trent's suggestions. Not having an initiate roll option always slightly triggered me, but that's more of a personal issue than a mechanical one.

Edit - of course, this means thaumaturge dedication moves to the v12 release of the EV module, and I'm OK with that.

@TikaelSol
Copy link
Collaborator

I still haven't had time to actually look through and figure out what this is doing or even how thaum works beyond what I half remember (not much at this point, brain is completely fried). So mostly it's down to me just being unable to dedicate time for a while. If one of the other devs wants to hit merge feel free to.

@mysurvive
Copy link
Contributor Author

There is actually a bug with this that I just found. It looks like the grantItem rule elements on the implements don't apply until the next update when there is a predicate in place. I'm not sure if this is something that can be worked around, but I'm looking into it.

@xyzzy42
Copy link
Contributor

xyzzy42 commented May 18, 2024

Maybe the ItemAlteration REs are running after the GrantItem REs?

@mysurvive
Copy link
Contributor Author

mysurvive commented May 18, 2024

I thought so too, so I changed the dedication GrantItem predicate to ["feat:implement-initiate"] and it still didn't update until the next cycle after the feat was applied.

Edit- the predicate on the Amulet (to test), not the dedication feat. That was a mistype.
image
the [class:thaumaturge] predicate actually does work, I'm assuming because it exists in the rollOptions prior to the predicate being run. I'm guessing predicates run earlier in the update cycle than other parts of rule elements do.

@mysurvive
Copy link
Contributor Author

Actually, it does work if you close the sheet, then re-open it. I guess the sheet re-renders weird.

@mysurvive
Copy link
Contributor Author

mysurvive commented May 18, 2024

This will require a change which allows ItemAlteration to run during preCreate, per Supe (which he has no bandwidth to work on until sometime after the v12 release). Currently, everything works hunky-dory except the Dedication, which will require closing/re-opening the sheet once the Implement Initiate feat is granted.

Conversation starts here: https://discord.com/channels/880968862240239708/880969304365994034/1241190711504932935

@mysurvive mysurvive force-pushed the Update_Thaumaturge_Features branch from e056d06 to f095085 Compare May 18, 2024 04:04
@mysurvive
Copy link
Contributor Author

Implement Initiate has been given the Item Alteration rule instead of directly on the feat per some recommendations in discord (https://discord.com/channels/880968862240239708/880969304365994034/1241244156635119696, https://discord.com/channels/880968862240239708/880969304365994034/1241244150230290513). This may or may not be acceptable since it adds another actor flag from the dedication feat.

This will work better (won't need to close/open the character sheet) if a trigger is added to run re-evaluates when feats are inserted (per Supe https://discord.com/channels/880968862240239708/880969304365994034/1241255907422109787)

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.

None yet

3 participants