pf2e icon indicating copy to clipboard operation
pf2e copied to clipboard

Published 20 hours ago • verified publisher icon foundryvtt

Restore secondary spellcasters for rituals

Open Cuingamehtar opened this issue 11 months ago • 10 comments

After attempting to correct the number of secondary casters for Awaken Portal ritual #14043 (that PR was not enough for it to work, which I somehow forgot to check), I wanted to correct the values for other rituals.

What I figured out was that in one of the migrations this data was stripped completely, and the field expected a numerical value. Since the system doesn't do anything with the number of secondary ritual casters at the moment (as far as I'm aware), and the values there are not as straightforward as simple numbers or ranges, I propose changing the field into string type.

The comment on the data type (about the maximum number of spellcasters) was also incorrect, as that was the number of required spellcasters.

Cuingamehtar avatar Mar 03 '24 09:03 Cuingamehtar

I don't think we're going to return it to text.

stwlam avatar Mar 03 '24 09:03 stwlam

This is reasonable. How about {minimum:number, maximum:number | null, details:string}? Where null represents an optional upper bound?

Cuingamehtar avatar Mar 03 '24 09:03 Cuingamehtar

That sounds good. Use min and max as shorthands, which is in use elsewhere. If you'd like to continue using this PR, I'll reopen it.

stwlam avatar Mar 03 '24 10:03 stwlam

Yeah, I want to continue with this PR.

Cuingamehtar avatar Mar 03 '24 10:03 Cuingamehtar

Here is how the Details tab currently looks image image image

I have not attempted the migration of the imported data yet.

Cuingamehtar avatar Mar 03 '24 18:03 Cuingamehtar

I think, I'm ready for another look.

I'm pretty confident about this migration - I've checked all rituals post-migration and the numbers match those in books/AoN/Demiplane.

I'm, however, concerned about migration 882. I had to modify it, since the type itself is now different, and tried to adapt it, but I'm much less confident since I'm not sure how to test it properly. Ideally, I think, it should make the type that is was making before, but I can't figure out how to define it in the file.

Cuingamehtar avatar Mar 06 '24 20:03 Cuingamehtar

I might have just been overcomplicating things with 882. Right now the results should be the way they were before, if I understand correctly.

Cuingamehtar avatar Mar 07 '24 19:03 Cuingamehtar

To make reviewing easier, go ahead and remove the migration result from the PR until it's otherwise approved.

stwlam avatar Mar 09 '24 03:03 stwlam

I've reverted the migration and rebased the changes on top of the 5.14.1 release version.

Cuingamehtar avatar Mar 10 '24 13:03 Cuingamehtar

Rebased on 130a83cc Added Diabolic (prev. Infernal) pact and Demonic (prev. Abyssal) pact Incremented migration counter

Cuingamehtar avatar Apr 07 '24 07:04 Cuingamehtar