pf2e
pf2e copied to clipboard
foundryvtt
Add support for modifying Infused crafting batch sizes
Generally intended to help with support for the 10 batch size for Munitions Crafter (https://2e.aonprd.com/Feats.aspx?ID=3158). It is implemented as a rule element to be put on the feat.
To make it work i also had to add type:<consumableType> to the getRollOptions list, to help filter down to the ammunition.
{
"key": "CraftingInfusion",
"selector": "munitionsCrafter",
"predicate": {
"all": [
"consumable:trait:alchemical",
"consumable:type:ammo",
"consumable:level:0"
]
},
"batchSize": 10
}
Is there anything else this is good for? For example, can this handle the witches cauldron feat?
A rule element needs to be as general as possible, otherwise hardcoding (such as what handwraps do currently) is preferred.
Is a separate rule element for this needed? What made expanding CraftingEntry undesirable?
Regarding the witches cauldron feat, the rule could be added to that one as well. It depends on how the feats are supposed to interact, because advanced alchemy explicitly states a batch of 2.
Making it a rule element may well have been overkill.
The rationale of doing it that way was to make it able to be independent of the CraftingEntry, so later feats, although i don't know that any exists currently, could modify further.
The concern would be that if it can't be CraftingEntry and must be its own entry, making it a rule element that can alter the quantity of a crafting attempt, whether its infusion or regular crafting, might make it more generally useful. That way you don't have a rule element for only one thing.
In general we need to take another look at the CraftingEntry rule element though, because the batch size is definitely supposed to be handled by that. We might need to consider giving it tweaks somehow.
So especially for Witches Cauldron I didn't bother adding any additional automation as there was no need. There should be nothing stopping you from Crafting 6 potions at a time and crafting them has nothing to do with Crafting Entries
Forgetting that the same argument applies to munitions crafter....
If all you want is mundane ammo crafting, and want to take the easiest route to it, you could add "munitions-crafter" as a field discovery and assign to it, like the field discovery feats do. Then you can add hardcode hande the batch size.
Then later in the future we can have a real solution and migrate all of them to the real solution.
Munitions crafter this definitly applies to, since it uses the Crafting Entry system and "spends" infused reagents to craft batches of ammo. So I can definitely understand the need for this.
Field discovery could definitely be a workaround for now, pending some review of the crafting code in future yes 🙂
More so that you don't technically need automation for munitions crafter either. You can add 10 bullets to your inventory, give them the infused trait, and manually deduct the reagent. Granted its more annoying to do so than cauldron, where you only need to tick an arrow twice.
Field discovery is probably the least intrusive way to do it right now. An RE to change batch sizes would need to be applied to all the alchemist subclasses, not just munitions crafter.
That definitional makes sense, the only thing i wonder is if we will get some wonky behavior for an alchemist with gunslinger dedication, who can effectively have multiple field discoveries then.
Oh don't worry, that already has wonky behavior. But field discovery is assigned via a path, so you'd assign the field discovery to the munitions crafter entry, not the alchemist one, and it'll be fine.
I updated the code in the direction discussed. One thing that caused some issues was the summation.
Math.floor(fieldDiscoveryQuantity / fieldDiscoveryBatchSize) +
Math.ceil(((fieldDiscoveryQuantity % fieldDiscoveryBatchSize) + otherQuantity) / batchSize)
Gave 1 more than expected, so I changed it to be simpler, although I worry that I broke some edge case I have not thought of by that simplification.
Interestingly it is only when the batch size does not match exactly to the field discovery batch size.
The field discovery was 10, and the default one was 2. In this case the cost goes up by 1 every other increment, as if it has a batch size of 2, until it hits 10, where it resets to 1, and then continues with every other from there, until it hits 20.
you'll probably need to special case ammo to default to the batch size (or perhaps all newly added items can do that?). But also I guess make sure only mundane ammo is affected. What you did seems to affect all ammo.
And yeah, it'll need to handle everything under the batch size as only using one reagent as well.
Looks fine to me mostly, only gotcha is that if you're going to add min level and mundane restrictions in the RE instead of in code, it might better to make that a predicate. Then you'd predicate on "item:level:0" and also probably not: { ["item:trait:magical"] }. You can give it item.getRollOptions("item") to predicate on in the meantime.
A magical check is probably not required if you're doing a level 0 check, that was my mistake. Safer to do it though I guess, since its only "basic" ammo.
Hmm, on second though, a predicate could also predicate on the item type. If we're going that far, we might as well add a migration for the other field crafts and convert them all to "predicate + batch size" things. And then we'd need a migration for all the existing ones. And perhaps it'd be better to do a new CraftingBatchSize RE with selectors for either mundane or infused crafting.
Since a migration would be required if we plan to use predication, I'm down to merge this as is, and we can revisit this later if they ever make more feats of this type in the future. Since this is pertaining to REs, are you ok with that @stwlam ?
Alright, got something more decisive now. Remove the two new properties you added, maxFieldDiscoveryItemLevel and fieldDiscoveryRequiresMundane. Instead hardcode the ammo field discovery batch size tweaks to be mundane only in the code itself (which was my original suggestion). Zero future proofing. There's no need to add configurability on something that will get the cattlegun later.
Also, if possible, post a video of it at work once you're done. Some things are harder to gauge from images. Play around with it if you can. If you can't record it easily nor have the time l'll check it out locally and try to break it myself.
For someone else (not necessarily you) that is sufficiently motivated in the future, my thought right now is to introduce a CraftingQuantity RE that uses a predicate to match a valid item, and has a field to determine if the crafting is regular or advanced. This will handle alchemist field reagents, munitions crafting, and cauldron, and any other "you craft X instead of Y" or "you make a batch of X per reagent" feature in the future. That'll require a migration, so I'd rather not force you to do that just to get munitions crafter working for your game.
You'll need to work with: https://github.com/foundryvtt/pf2e/pull/3424 once it's in. Should make things easier.
I skipped the migration on my PR to do it here instead once everything is sorted for munitions crafter
If this PR ends up with the hard coded approach for now, and at a later point it will be done with something like what Carlos suggested a craftingQuantity RE, then it seems i don't need to touch the RE at all for now.
Although i am not entirely sure the approach with the hard coding i have taken was what you guys were thinking. If it was not this direction i can adjust it.
I'm busy currently, but from a glance it looks fine and its more or less what I suggested. I can dig more later.
No more move on this ? Ammunition crafting is still a chore without it :innocent:
@Gronex Are you able to rebase this with the newest crafting changes and continue?
If not I can pick this issue up
I won't be able to take a look until Tuesday at the earliest, so you are welcome to pick it up
The potential solution for this is to turn Field Discovery into a Predicate, allowing Machinist to function as expected with a field discovery batch size of 10.
You hit another problem when you get to Precious Munitions, as there's currently no way to support 3 different batch sizes in a single entry. And there's currently no way to set precious materials on ammunition.
Since munitions crafter and it's feat tree seem rather unique (at least for now), Precious Muntions could make another crafting entry that only accepts precious material ammo, with a batch size of 1.
Either that or a more sophisticated way of calculating batch sizes for crafting entries, which feels unnecessary to support a single feat.
I suppose with #3787 there is no longer a need for this PR, as that one handles the same in a much nicer way, as far as i can tell.
@Gronex since #3787 was dropped can we reopen/merge this? Ammunition crafting is still not working correctly as far as I'm aware
@DawidIzydor I am not really sure why the other one was closed, so to have any real hope of getting it merged that may need to be clarified. Regardless I think we might be better of starting over, since there has been a fairly large amount of restructuring in the crafting area since this PR was abandoned