Item Sheet Details Reorganization

[old wind]

Starting a thread to keep discussion organized

[old wind]

@glass jay @foggy sail are all three of these fields related to critical damage?

[foggy sail]

No, those are unrelated

[old wind]

Okay, I'll pull them out of critical damage. Is there a better title/header for these that I could use?

[foggy sail]

Those can probably go under the activation details or somewhere like that

[old wind]

Cool, I can move those fields to the activation partial

Any idea whether that'll break anything?

[foggy sail]

Iirc those fields only show up if the feature has an attack type (melee/ranged attack, or save etc). Either putting it there instead or decoupling it from that would work

[old wind]

Is the "Other Formula" an alternate damage field?

Or does it override something if it's filled in?

[foggy sail]

Other formula is just a miscellaneous additional formula

I don't think we use it much, but it's a very generic field

Maybe it can stay in damage

[old wind]

Still in progress, but this is kind of what I'm thinking in terms of organization

[foggy sail]

One issue I've had for a while is showing the weapon properties section on non-weapons. Things like augmentations and various magic/hybrid/tech items have attacks and need a weapon properties section, but only showing the section if it has an attack type looks odd, since weapon properties is above (or in this case, on a different tab to) action details. I'm not really sure how to lay that out to be more natural

[glass jay]

I can't remember, is the critical damage field still erroneously including modifiers?

[foggy sail]

Critical damage adds damage on a crit

[glass jay]

Ya it shouldn't do that

[foggy sail]

I mean, that's what it's set up to do, so it's not a bug at the very least

[glass jay]

Ya I get that, originally dnd code and such

[old wind]

I feel like I saw there was a pull request to add those somewhere, is that in progress?

[foggy sail]

That PR just adds the section to more item types. I put it on hold to decide how to integrate it properly. Consider it superseded by this

[old wind]

Gotcha

For augments etc. would they need to have the entire weapon properties bubble (I'm calling them bubbles), or just the section with all the properties checkboxes?

[foggy sail]

The whole thing

Iirc they already have the "proficient", "equipped" etc line, but they need weapon type/category/properties too

[old wind]

Okay I'll have a look at it. I'm planning on going through the rest of the item types once I finish weapons to give them all a consistent look

Don't want this to be a thing 😬

[foggy sail]

That doesn't look too bad 😆

Fwiw though I really do like the styling on the bubbles. Makes it feel a lot less cramped

[old wind]

Yeah thanks, I think it'll make it easier to dive into the details tabs on the items

[old wind]

What's the reason that the armor sheet template is equipment.hbs?

legacy stuff?

[foggy sail]

Armor and equipment are sometimes used interchangeably for some reason

[old wind]

If I change that sheet around to make the armor sheet cleaner, do you know if that'll bork anything else using that template?

[foggy sail]

I think the armor item type is called "equipment", which means the template needs to be named as such

Look at the template getter on sheet.js

Changing the layout of the sheet will be fine though, it's self contained

[old wind]

Cool, sounds good

[old wind]

I'm finished a first pass with all the equipment items. Need to have a look through other stuff (classes, spells, starship parts, etc.) to make sure the new partials don't add too many of these instances. Will probably get to it later this week.

[old wind]

I think the damage operator box and healing damage type checkbox may be broken for vehicle attacks

Neither box seems to persist when selected/checked

This is on v23

Has anyone else seen this issue?

[foggy sail]

Probably not, no one uses vehicles 😆

There's probably a bug in vehicles that makes your computer explodes, but no one has discovered it yet

[old wind]

lol

[foggy sail]

Damage operator is pretty old at this point anyway, not sure why healing fails

[old wind]

The exact same template/handlebars looks like its used for weapon critical damage, which seems to work fine is also broken

[foggy sail]

Then its something to do with the data paths used to render the template/save the data

[old wind]

correction, it's actually also broken on those, I think

Yeah okay it's also definitely broken on weapon critical damage sections

[old wind]

For containers, it seems like their equipment status section doesn't use the item-status.hbs partial, it's coded into the sheet. I was going to replace that but I noticed that they have an "attuned" property that I haven't noticed appears anywhere else

Is that meant to be there, or is it something legacy that can be removed/updated with the standard partial?

[foggy sail]

That's definitely a legacy thing, How'd that survive so long

[old wind]

Lack of use/notice I guess haha

I'll update it

Containers should just need the three checkboxes for status, right?

Nothing special?

[foggy sail]

Yes, if that matches the container data structure

[old wind]

Cool, yeah that matches template.json for the container item

[old wind]

I've seen a few of the starship sheets have this passed into the common component data partial, but it seems unnecessary to me. Does doing that serve any purpose?

I've tried deleting it and it doesn't seem to break anything

[foggy sail]

this acts as the data object to use to render that template, as if it was the result of getData. I imagine Handlebars implicitly passes this by default though

[old wind]

Ah I see

After some extra googling, it looks like explicitly passing this is "safer" in case the Handlebars are compiled with the explicitPartialContext option set, but it looks like foundry doesn't do that by default

Since no other item sheet partials pass this, I'll remove it from the starship partials for consistency with the other sheets

[old wind]

All finished with the sheets. I've changed the PR from draft to ready for review.

[fossil anchor]

Just wanted to say awesome work @old wind that will make using those a much nicer experience.

[old wind]

Thanks!

[old wind]

I think I'm done with fixing this up to resolve conflicts with the latest development changes

[old wind]

Going back through the thread I realized this is something I never resolved. Do you want me to include it in this or save it for a separate pull request?

[foggy sail]

That can go in this PR

[old wind]

Cool sounds good

[old wind]

Regarding this, I had previously moved the weapon properties to their own partial that renders in a bubble like image 1 whenever you insert it, in this case on a weapon. I'm thinking of two options, number 1 is easier but probably not as nice while number 2 is probably better but will require more work. 1 can also function as a temporary placeholder while I work on 2.

1. I could implement a check to render the partial within the Feature Attack bubble at the bottom (image 2) if the action type is a weapon attack and the parent item is not a weapon.
2. Since the weapon properties is a huge block of checkboxes, I could make the property selector an app, and have it pop out when clicked (like movement on character sheets does).

[foggy sail]

I have always wanted to extract the weapon properties boxes to an app

It'd be an excuse to tune up the trait-selector app too

We use that for languages and it could really use a search box

[old wind]

Yeah I think that'd be better overall. Could I copy-paste the trait selector app as a starting point or would it be better to start from scratch?

[foggy sail]

I think we could use trait-selector itself, or at the very least subclass it

But either way, trait selector needs some touch ups

[old wind]

Would it be better for me to wait for you to do that first?

I can also give it a go (in a separate PR) if you're busy with other stuff

[foggy sail]

Feel free to take a stab at it

[old wind]

Cool, anything you'd like to see included other than a general refresh of the look and addition of a search bar?

[foggy sail]

As a stretch goal, I'd like some framework for allowing input elements associated with each checkbox, so we can do store values for weapon properties (e.g explode 5ft)

[old wind]

Item Sheet Details Reorganization

[old wind]

Would it be better to add "properties": {} to the action template in template.json for every item that has an action, or to conditionally add it in via code it the action type is selected as melee weapon or ranged weapon attack?

[foggy sail]

How is it done now?

[old wind]

Right now in template.json, properties shows up directly under weapons and shields, it's not included in any of the templates

For weapons, it's

properties: {}

For shields, it's

"properties": {
  "nonlethal": true
}

[foggy sail]

That can be done in template.json

At some point I wanna turn that into a Set, so we don't need 100s of falses, but that's for another time

[old wind]

Cool, that seems easier to me

[old wind]

I've updated the PR with code to add properties to the action template in template.json, and for now added the properties to the action partial conditionally. If the item is a weapon or shield, it doesn't appear. If it's anything else with an action, it shows up.

When the trait selector gets tuned up, this weapon-properties.hbs partial can basically get replaced anywhere it exists with a button that opens it to select weapon properties. For now I think it's fully functional, though.

[foggy sail]

Why does it not show for weapons and shields?

[old wind]

For weapons and shields it shows up on the “Properties” subtab instead of within the action

That was more in line with where it is in the current sheets. I can just move it under the action for all sheets for consistency

[foggy sail]

No that works well

[old wind]

Cool

Additional thought on weapon atttack stuff, do weapons ever use any of the fields under Weapon Usage other than Range Increment?

[foggy sail]

Maybe 🤷

I wouldn't discount it

I've always wanted to pull range increment out of that section though, since it requires having a "none" activation on every weapon

[old wind]

Yeah I was thinking it's a bit messy having all that info displayed if in 90%+ cases you don't need it.

[foggy sail]

Some are set to standard action too, which causes all sorts of issues

[old wind]

I could slide it down to Weapon Attack conditionally if the item is a weapon or shield

I think it needs to be in that partial for spells, maybe other stuff

@glass jay would also be interested in your opinion on it

[glass jay]

let's see, for the weapon usage field?

[foggy sail]

Spells would need it on the activation partial, yes

[glass jay]

First note I have is we've never really needed Activation conditions. most I used it for was a spot to place RP cost. But that always seemed outa place

[old wind]

Yeah the idea would be that range increment would get pulled down to the Weapon Attack section if it's on a weapon and stay where it is for everything else

[glass jay]

Do agree with lebomb about sneaking out range. Or a possibly more useful solution, for more then weapons, is an Activation cost option for "Passive".

So all the options still appear but the weapon/(feature) still displays as passive

[foggy sail]

That's effectively what "Other" or "None" have been used as historically

[glass jay]

as far as features, those options still change them into active feats

And should continue to, for all the active features I've set with "none"

Can this menu not be displayed with a "_" option selected?

I understand that's likely a mess to rig

[foggy sail]

Not sure what you mean by that

[glass jay]

you know how most of that menu doesn't appear til an activation cost is selected?

Why not have it always there is what I'm saying

[foggy sail]

At the moment, it's primarily a clutter thing. But tabs may make that more feasible

[old wind]

What if I set it up such that it appears under weapon attack if no activation is selected?

Basically if there's an activation, it looks as it does now (right). If no activation is selected it "hops" to the attack section (left)

[glass jay]

I like that

[old wind]

I had it move Area as well because I think some flamethrower weapons use that field

[foggy sail]

Area is used for the explode property too right now

Don't remember off the top of my head, don't grenades use limited uses?

[glass jay]

Yes, but they don't consume like Consumables do

[foggy sail]

Right, so they do use limited uses

[glass jay]

The consume check box should be added though

As an addition to that. Think it's possible for an easy sheet Quantity adjuster? Seen at least one user againts the Grenades consume. But their reason is for players "using" their grenades before wanting to undo that choice

[old wind]

I can tack that in as well

Actually, grenades use the saving throw field as well, so at that point it's kind of moving everything over

At which point might as well always display everything

[foggy sail]

I mean, saving throw is already there, but that is my concern

Both grenades and flamethrowers could be seperated from limited uses and area via other system improvements

[old wind]

Alright so for now, this is what I have

In this case any weapon other than grenades should be fine with null selected as the activation. I bubbled the Range Information to make it clearer to a user that it's moved if they change the activation type

[old wind]

It might be good to change the None option to something like Passive to go along with this

Then it's a bit more clear that the grenade has an activation, but you don't have to do anything for it

[glass jay]

Not quiet.

[foggy sail]

None also represents "Free actions"

Even though they don't exist RAW

They are simply Not an Action

[old wind]

Ah

[glass jay]

None allows the "Activation" button for features that need it too

[old wind]

Gotcha

I'm going to leave it like this for now, I think this'll make it cleaner for 90% of weapons without breaking anything. We can update stuff in the compendiums to change all the non-grenade weapons to activation cost => null, or we can ignore it, everything will function fine either way.

If anything's done to change how grenades and flamethrowers are set up I can make more changes as needed.

[old wind]

I can add this but it might require some extra work to get it to be functional. Will make a note of it, though

[old wind]

Would this be something on the item sheet, or on the character sheet in the inventory tab?

[glass jay]

inventory tab

item sheet can already edit the quantity

[old wind]

I almost have the trait selector redone, but I have a weird bug I can’t seem to fix

It works perfectly for everything I use it to select/deselect, except if I import one of the pregen characters from the compendium. In that one instance, I can’t deselect any of the weapon or armour proficiencies that the character has at the time of import from the compendium

Like if an iconic has basic melee weapon proficiency, I can add and remove advanced melee weapon, heavy weapon, whatever I want proficiency. BUT I can never get it to remove basic melee proficiency

[old wind]

Okay I think it's because I removed and forgot to replace the backwards compatibility trait parser
Nope it's not that

It's much simpler than that. Each of the imported characters has a class which grants proficiencies. You can't remove those with the trait selector, they just go back to being selected when the actor's data is prepared.

The bug was a feature the whole time

[fossil anchor]

Lol

[old wind]

It did make it apparent that some way of indicating what is set by a class (or other items) should DEFINITELY be indicated on the form somehow.

Wouldn’t want a user to get as frustrated using it as I did writing it 😅

[old wind]

Following on from this, actually, is there a good place to hook into the setting of actor proficiencies and other traits and pull that data into the form so I can mark those choices as not editable?

I think I could get that from the actor sheet when they're calculated and then store it somewhere on the actor, maybe in flags if that makes sense?

[foggy sail]

I'd try to avoid storing anything on the actor, especially persistent data like saving it to the database

[old wind]

Would it be better to store them on the actor sheet?

[foggy sail]

There's no real concept of "storing" something on a sheet, it'd just be fetching it in getData

Even storing it on the class only lasts as long as the class is in scope

[old wind]

Okay I think I see what you mean

[old wind]

Do you know where the code is that checks the proficiencies from a class item and grants them to the actor?

I've been poking around but can't seem to find it

[foggy sail]

That'll be in the rules closures

calculate-weapon-profs or something

[old wind]

Alrighty, I have it checking for and disabling user-toggleable checkboxes for proficiencies given by a character's class(es), and added a tooltip explaining it.

Can't procrastinate and put off migration code any more...

[old wind]

I've changed the data format for properties to support a text "extension" field for each weapon property. Instead of the format {explode: true/false} it's stored in the format {explode: {value: true/false, extension: '5 ft'}. I've updated the code on the item sheet everywhere it references properties in the old way and it seems to work fine on the item sheet and in the trait selector. However, it's not working on the chat cards. I saw some code on the item.js file to generate that data, which I updated, but it's showing all of the properties instead of just the selected ones still.

Is there somewhere else outside of the item that that needs to be adjusted as well?

Found it, in dice.js

[old wind]

There's some code in migration.js for updating unlinked tokens in scenes, but I think it may be broken in V11

It looks like unlinked tokens get updated by migrateActor()

[foggy sail]

This file is pre-v10 😂

[old wind]

Yeah I figured, there are lots of angry yellow "please reference actor base data directly" messages in the console lol

I don't want to butcher it as it still needs to work for those older versions, but I could put a check in there to only run that part of the migration if the current system version is v0.24 or higher?

Then it should still work for anyone running v10, and not double-migrate actors for those already on v11?

[foggy sail]

It doesn't need to remain at all. It should be brought up to date

If someone comes from say, V9 to V10, Foundry migrates them to the V10 format. There is then no need for the V9 format

[old wind]

Ahhhhh true true true

Okay cool, I'll slice it up and make it look nice, then

This should be the last thing I need to do before the trait selector is good to go, unless I'm missing anything.

[old wind]

@foggy sail Alrighty, I think it's done for now. I've marked it as ready for review whenever you have time to look at it.

[old wind]

Reviving this dead thread. I’ve finished the updates to the trait selector I was making forever ago. The update changes weapon properties to use a new and slightly modified data format, which requires migration. The selector is backward compatible with reading the current data format but will always write using the new one.

I think at some point it was suggested that making a separate node script and PR to migrate compendium actor/item data would be the best course of action to simplify review. @foggy sail is that still the preferred course of action?

[old wind]

@foggy sail I saw that the PR was merged for this today, as a result I want to deprecate the module version by listing compatibility only with versions of SF that don't include the new sheets in the system. Is the plan for this to be in a 0.26.x release, or a 0.27.0 release?

[foggy sail]

The new sheets will be in 0.27