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 starfinder ammo reloading #30

Merged
merged 23 commits into from Nov 9, 2022
Merged

Update starfinder ammo reloading #30

merged 23 commits into from Nov 9, 2022

Conversation

SoxMax
Copy link
Contributor

@SoxMax SoxMax commented Nov 5, 2022

I've changed out the reload window to act more like the inventorylist but filtered to only ammunition that "isn't loaded". Upon loading ammunition the ammo will have its location changed to the weapon its loaded into.

Currently there's no checks that the "correct" ammo is being used. But batteries are handled differently from cartridges since you can't transfer charges like cartridges.

This probably isn't ready to merge, but wanted to get eyes on it to make sure I wasn't barking up the wrong tree with this direction to take things.

@bmos
Copy link
Owner

bmos commented Nov 5, 2022

This looks pretty neat! What are the downsides/why not merge?
It seems like it might make a lot of sense for SFRPG.

@bmos
Copy link
Owner

bmos commented Nov 5, 2022

One weird thing: The "load ammo" window opens behind the character sheet.

@SoxMax
Copy link
Contributor Author

SoxMax commented Nov 5, 2022

For not merging yet there's a few little things, like the window opening behind the char sheet. The way I'm detecting loaded ammo is by if the location field is populated, which could also be used to put ammo in say a backpack or bag of holding type item

@SoxMax
Copy link
Contributor Author

SoxMax commented Nov 5, 2022

There's also weird edge case things where a user could change the location field of some ammo to a weapon manually and think their ammo is loaded, but the only way to actually load ammo is through the reload window. But that may be fine and these changes are good enough for now to go live and can be built upon

@SoxMax
Copy link
Contributor Author

SoxMax commented Nov 5, 2022

I was able to fix the generating window behind character sheet problem.

@SoxMax
Copy link
Contributor Author

SoxMax commented Nov 5, 2022

I was thinking more too and this is probably safe to merge now, users can always disable the ammo linking if they want and I think it at least provides more functionality than there was before.

@bmos
Copy link
Owner

bmos commented Nov 5, 2022

why not just use a windowreference node?
https://github.com/bmos/FG-Ammunition-Manager/blob/main/campaign/scripts/ammo_ammopicker.lua#L33
https://github.com/bmos/FG-Ammunition-Manager/blob/main/scripts/manager_ammunition.lua#L18

that should fix any weirdness you might run into trying to match strings from the location fields

@SoxMax
Copy link
Contributor Author

SoxMax commented Nov 6, 2022

Oh yea I'm using windoreferences when you load ammo from the reload GUI. My concern is more around someone manually set the location of some ammunition items in their inventory to be one of their weapons thinking that will also load the weapon, when it won't do anything as far as this extension is concerned.

@SoxMax
Copy link
Contributor Author

SoxMax commented Nov 7, 2022

So I did some digging, it looks like to deal with manual updates to the location field I'll have to make some major changes to the inventorylist. So I guess we'll see if users fall into it very often, I'm hoping not since the reload flow normally is from the actions tab.

@bmos bmos merged commit 331cb6b into bmos:main Nov 9, 2022
@SoxMax SoxMax deleted the starfinder-container-ammo branch November 11, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants