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

Add new Ship Equipment Display implementation #5315

Merged
merged 10 commits into from
Jan 14, 2022

Conversation

Web-eWorks
Copy link
Member

Add a completely refactored ship equipment display to the Ship Info screen and the Equipment Market screen. This is a major step towards both refactoring the equipment system under the hood, and work @Gliese852 is starting to do with equipment integrity/durability. It's also just really pretty, thanks to @nozmajner's wonderful mockup work which this screen is based on! 😄

I'll let the images speak for themselves, as I've been working on this branch for a very long time today.


Note: this branch is based on the BBS refactor branch, and as such will be rebased to clean up commit history once that branch is merged. Anyone wanting to read the diff can start with bbcc7fb (Add lang keys for additional mass units) to get a shorter version of the branch changes.

@impaktor
Copy link
Member

impaktor commented Jan 4, 2022

Looking very slick!

@Bodasey
Copy link

Bodasey commented Jan 4, 2022

Nice improvement of the look! Good work!

But... (sorry for being a hard tester)

  • in the Equipment market, mark an installed equipment
  • purchase another one

before_overwrite
after_overwrite

The marked equipment is replaced / overwritten!

Maybe the equipment market could be more intuitive if it had more the look and feel of the commodity market, right and left side could change places, especially purchasing does still not need confirmation (see #5020); I think it's even a bigger need for confirmation here than in the commodity market because purchase and sell price differ (wrong purchase and resell results in financial loss).

Further, the category "sensors" is empty, radar and target scanner are classified as equipment; is "sensors" a placeholder for something invisible/future?

@impaktor
Copy link
Member

impaktor commented Jan 4, 2022

But... (sorry for being a hard tester)

Don't feel sorry, it's GREAT to get our PRs tested. MUCH easier to fix bugs on PRs when code and motivation is fresh in the mind, rather than 6 months later in master, where people encounter it in stable release.

And all time used by players to playtest PRs is time the developer can use develop more cool stuff.

@Web-eWorks
Copy link
Member Author

The marked equipment is replaced / overwritten!

That's on purpose. The UI is designed around a slot-based system; you select the equipment in the slot and have an option to either replace or sell it. The UI doesn't do the world's greatest job of communicating that fact though, so I may try to improve that with an indication that you're purchasing a replacement equipment.

Especially purchasing does still not need confirmation (see #5020); I think it's even a bigger need for confirmation here than in the commodity market because purchase and sell price differ (wrong purchase and resell results in financial loss).

I ran out of time yesterday to implement that - it requires a deeper refactor of the market widget than I was able to get into at the time. This is definitely a goal I would like to achieve.

Further, the category "sensors" is empty, radar and target scanner are classified as equipment; is "sensors" a placeholder for something invisible/future?

Yes, the sensors category currently only contains the planet scanner equipment, due to the underlying existing slot system. I may shuffle things around slightly to move more slots into that category, but a lot of this UI is setting a target for future work on the underlying slot system.

But... (sorry for being a hard tester)

I definitely appreciate the testing and feedback! I already knew most of the reported issues were going to be pain points for users getting used to the new system, but it's good to get confirmation (and indirectly, confirmation that nothing's crashing/seriously broken).

@Bodasey
Copy link

Bodasey commented Jan 5, 2022

  • sold or replaced equipment is not put to the stock, but lost (or scrapped?), finance calculation is correct (47 cargo bay life support in the screenshots above)
  • equipment can be sold in ports without sufficient tech certificate and is lost (at least, you get the money). Not sure how to handle that: Can it be stored at all? What if you want to downgrade a class 6 hyperdrive to a class 4 one? Only possible in a certificate >=7 port?

@Web-eWorks
Copy link
Member Author

I knew I was missing something here - thanks for catching that the station stock wasn't being affected by selling. Selling equipment in ports without certification is a thornier issue and not one that I believe we have a concrete solution for. We don't (yet) have any way to store purchased equipment in a starport for use on different ships, although that might be a possibility in the future.

@Bodasey
Copy link

Bodasey commented Jan 10, 2022

Idea for selling over-tech equipment:

Message "We are not authorized to take this equipment. We pin a bulletin for you (only in background), maybe someone else here is keen on your device?"

probability for finding a customer may be (port_tech / equipment_tech) / 3 or ((equipment_tech - port_tech) / equipment_tech ) / 5 or completely different.

(first formula: the less difference, the easier to find a customer
second formula: the more exotic the device, the bigger the probability to find someone who looks for it)

If one is found, another message appears: "Device .... sold for amount ... "
else "No customer found, you have to keep your device ... "

- Break out ship equipment by slot type
- Detailed equipment display with quick stats for each item
- UI paradigm is forwards compatible for physical-slot equipment model
- Replace existing equipment market screen with new filtered market
- Integrate new equipment widget into ship info screen
@Bodasey
Copy link

Bodasey commented Jan 11, 2022

put back to stock not fixed yet

launch button vanished from main window

no_launch

@Web-eWorks
Copy link
Member Author

put back to stock not fixed yet

No commits were pushed to fix it, how could it have been fixed?

launch button vanished from main window

Interesting, I don't think I touched that area of the code at all. My guess is pre-existing bug, but I'll see if I can reproduce it.

- Add ui.button wrapper with themed padding and color variant support.
- Setup default button colors to use Pioneer theme style.
- Add ui.getButtonHeightWithSpacing(), ui.alignTextToButtonPadding().
- Expose current ItemSpacing value as ui.getItemSpacing().
- Engine.pigui now available when loading themes. Other Engine methods not guaranteed,
- Refactor category displays (radar, scanners in sensors category)
- Add context information when replacing selected equipment
- More documentation
Non-integer values (more specifically sub-pixel offsets) produce blurry results with UI icon drawing and calculations.
ui.rescaleUI defaults to snapping result values to the next whole number.
@Bodasey
Copy link

Bodasey commented Jan 11, 2022

Update station stock when exchanging equipment

fix failed: same stock amount when selling, and on replacement, you get

Warning: [string "[T] @pigui/libs/ship-equipment.lua"]:139: attempt to call method 'sell' (a nil value)
stack traceback:
[string "[T] @pigui/libs/ship-equipment.lua"]:139: in function 'onClickBuy'
[string "[T] @pigui/libs/equipment-market.lua"]:52: in function 'buy'
[string "[T] @pigui/libs/ship-equipment.lua"]:132: in function 'onClickItem'
[string "[T] @pigui/libs/table.lua"]:141: in function 'fun'
[string "[T] @pigui/libs/wrappers.lua"]:235: in function 'child'
[string "[T] @pigui/libs/table.lua"]:87: in function 'fun'
[string "[T] @pigui/libs/wrappers.lua"]:616: in function 'withStyleVars'
[string "[T] @pigui/libs/table.lua"]:86: in function 'render'
[string "[T] @pigui/libs/equipment-market.lua"]:237: in function 'render'
...
[string "[T] @pigui/libs/wrappers.lua"]:90: in function 'window'
[string "[T] @pigui/views/tab-view.lua"]:116: in function 'fun'
[string "[T] @pigui/libs/wrappers.lua"]:648: in function 'withStyleColorsAndVars'
[string "[T] @pigui/views/tab-view.lua"]:113: in function 'renderTabView'
[string "[T] @pigui/views/station-view.lua"]:76: in function 'draw'
[string "[T] @pigui/views/game.lua"]:57: in function 'callModules'
[string "[T] @pigui/views/game.lua"]:240: in function 'fun'
[string "[T] @pigui/libs/wrappers.lua"]:90: in function 'window'
[string "[T] @pigui/views/game.lua"]:230: in function 'fun'
[string "[T] @pigui/libs/wrappers.lua"]:583: in function 'withStyleColors'
[string "[T] @pigui/views/game.lua"]:229: in function <[string "[T] @pigui/views/game.lua"]:216>

launch button vanished from main window

Interesting, I don't think I touched that area of the code at all. My guess is pre-existing bug, but I'll see if I can reproduce it.

see #5320; seems it has been reduced here to even one icon

@Web-eWorks
Copy link
Member Author

Update station stock when exchanging equipment
fix failed: same stock amount when selling, and on replacement, you get

Thanks for catching that! I've fixed that issue and (hopefully) caught all other cases where you can sell equipment from this menu and not have station stock update correctly.

@Bodasey
Copy link

Bodasey commented Jan 12, 2022

fixed

(name the button "Sell / replace Equipment", not "Sell Equipped")

@Web-eWorks
Copy link
Member Author

The sell button specifically only sells the equipment in the slot - clicking on items in the list purchases them and does actually replace the equipment. The list header changes to indicate that replacement is happening, so I don't think further context cues are actually necessary or productive, given that future changes will involve adding a details/confirmation pane when purchasing equipment.

@Web-eWorks Web-eWorks merged commit 8001f97 into pioneerspacesim:master Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants