35 Replies
Ethaks
Ethaks2y ago
🧵
Mana
Mana2y ago
I hope I didn't miss any corner cases where it doesn't work besides the one that the MR had a comment about. Oh, the migration shim is probably bad, on quick glance. I simplified it but I think I broke it at the same time. .. Nope, it works. I was looking at old version of that.
Ethaks
Ethaks2y ago
I just created a world container item, opened its contents tab, and added a new weapon through the + sign. The Contents Weight then jumps to NaN from 0 I think that's a sheet and not a data issue though
Mana
Mana2y ago
I can't even create a new container in an actor... there's something definitely broken.
foundry.js:319 TypeError: Cannot create property 'currency' on number '0' [Detected 1 package: system:pf1] at ItemContainerPF.prepareBaseData (systems/pf1/pf1.js:20140)
Ethaks
Ethaks2y ago
Is that after a server restart to account for template.json changes 😛
Mana
Mana2y ago
Of course I forgot that.
Ethaks
Ethaks2y ago
I forgot about that when I checked out the MR, so I saw that error like 5 minutes ago 😄 And yeah, that contents weight stuff is a sheet issue, the hbs needs a data.weight.total I guess
Mana
Mana2y ago
That attempt may have broken my actor tho
foundry.js:319 TypeError: Cannot read properties of undefined (reading 'forEach') [Detected 1 package: system:pf1] at ItemContainerPF.getValue (systems/pf1/pf1.js:20254) at systems/pf1/pf1.js:17434
Ethaks
Ethaks2y ago
Does the Weight field in item sheets only show the .value now? I think it used to show the calculated overall weight (now .total) after multiplying with quantity.
Mana
Mana2y ago
It should show it like it did before, unless I hooked the wrong variables.
Ethaks
Ethaks2y ago
(Sorry if the points I bring up are fragmented, I sent the initial message because something seemed majorly broken, but that was my fault. So now I'm kind of abusing this as a stream of conscious, if you don't mind)
Mana
Mana2y ago
It's fine. I don't recall if I tested quantities correctly. It's been like a month now since I did the MR. The weight on the sheet was per item even before. Only price changed to represent the entire stack.
Ethaks
Ethaks2y ago
Ah, that might have been the case, my bad
Mana
Mana2y ago
At least that's the case in 0.80.24
Ethaks
Ethaks2y ago
One thing with quantities: Create world item container, open contents, add weapon, set weight to 10, set quantity to 2. Container weight is calculated correctly. The weapons weight.total is 10 however. Is that meant to be the case and the quantity including weight is stored somewhere else?
Mana
Mana2y ago
I'm not sure items ever stored stack total. I didn't change anything related to that, at least I don't think. Same happens with items outside of containers. I guess it should be multiplied by quantity, otherwise it doesn't do much besides .converted?
Ethaks
Ethaks2y ago
I'm pretty sure that there was supposed to be a value somewhere, although the weight update from preUpdate to derived might have broken that
Mana
Mana2y ago
Purely from design point, it should have the lbs value per single item, and lbs value per stack in derived data, with converted values for both. But it doesn't do that right now. Should I fix that on my end or are you fudging that into working order?
Ethaks
Ethaks2y ago
I'll probably not get started today/this evening. I'm okay with looking into this as well as with you wiring that up, so I'll leave that up to you.
Mana
Mana2y ago
I have time I can put into it.
Ethaks
Ethaks2y ago
That'd be great, thanks!
Leo The League Lion
@ethaks gave vote LeaguePoints™ to @manaflower (#19 • 130)
Mana
Mana2y ago
Okay, it's all done. As unintentional side effect the sheet now displays weight of the stack when not editing the value. So it matches the behaviour with price. Well, done for the most part. .total now is stack total. and .converted is the stack total converted. There's no converted value for singular item. Not sure what to do about that if anything. I guess converted could be an object with .value and .total of its own..
Ethaks
Ethaks2y ago
Thank you! I'll check it out again in ~10 hours.
Mana
Mana2y ago
Just FYI, I didn't to this part. I can do it, but I was uncertain if I should.
Ethaks
Ethaks2y ago
To avoid going totally silent on this: I did check out the MR again, and I've been tinkering a bit with it (going for the .converted object, adding tests, adjusting which values appear in input fields, container contents)
Mana
Mana2y ago
Alright.
Mana
Mana2y ago
I'm experiencing some lagginess with what was merged.
No description
Mana
Mana2y ago
This item is 10lbs, updating quantity doesn't update weight after refresh until after several changes. After it finally changes, it updates in timely fashion for all changes after. This was specifically item in a container in items directory. The container itself updates despite the lag in the item sheet. And this is repeatable after every F5
Ethaks
Ethaks2y ago
Not at a computer yet to check this, but what does the item's data contain? Is the weight preparation run and does its thing, is it it already stuck there?
Mana
Mana2y ago
Looks like it's in the derived data. So it's just not refreshing the sheet for the first several changes to it for unknown reasons.
Ethaks
Ethaks2y ago
Found the reason: the item's _onUpdate is never called due to an early continue in item/entity.js:850 when the item's data is not found in memoryVariables["data.inventoryItems"]. No _onUpdate -> no render call at all. This is just not as obvious with the quantity because that input field just stays like it is, whereas the value shown in the weight field is changed after leaving the input and depends on the render.
Mana
Mana2y ago
Nice investigative work there :)
Ethaks
Ethaks2y ago
I had to prep my own session so I left the cleanup to Fury, and it seems the description was not 100% correct as the actual point was a few lines below. Getting within three lines with a single breakpoint and less than 5 minutes is good enough in my book though 😄 Also thanks @manaflower for finding that issue. I definitely did not check for something like that, and I doubt I would've found that bug anytime soon.
Leo The League Lion
@ethaks gave vote LeaguePoints™ to @manaflower (#18 • 134)