OpenLoco / OpenLoco

An open source re-implementation of Chris Sawyer's Locomotion
https://openloco.io/
MIT License
1.26k stars 155 forks source link

Allow object selection window to list more than 1000 objects #1682

Open AaronVanGeffen opened 1 year ago

AaronVanGeffen commented 1 year ago

Currently, the object selection window, like other windows with a scrollview, is quite limited in how many entries can be displayed. Due to scrollviews using int16_t to keep track of their scroll positions, there is only room for about 16383px of information. In the object selection window, with a row height of 12px, that means only 1365 items can be displayed.

Now that all windows have been implemented (save for the legacy multiplayer window), we should try increasing the scrollviews capacity. Hopefully, nothing will break in the process, but we might need to reimplement a few more functions in C++ along the way.

memellis commented 6 months ago

Hi @AaronVanGeffen I will have a go at implementing this. Any tips?

AaronVanGeffen commented 6 months ago

Sure, this one seems feasible at present, though it requires changing the coordinate types of not just the scroll structs, but I think all drawing functions as well -- or at least the ones involved. Many of them use int16_t for coordinates; I think we should switch them to int32_t at least, though preferably taking this opportunity to group coordinates together using the Point or Point32 types. I'm not sure why we're using signed integers for these coordinates, to be honest.

I previously looked into this myself, though I merely scratched the surface by changing the types for ScrollArea and the getScrollSize event. The result of that can be found here; please feel free to use it: https://github.com/AaronVanGeffen/OpenLoco/commit/9587cdfc44579e004148ef26f8371bdaa0771848.

Tagging @ZehMatt as he might also have opinions on how to do this properly.

memellis commented 6 months ago

Great thanks for the tips. I'll have a play!

ZehMatt commented 6 months ago

There shouldn't be too much in the way of actually changing it. This will probably require a good amount of changing variables here and there as a lot of the variables are simply int16_t, I think it will just bubble up once you start hacking at it.

Edit: One thing that might be good to change first is to change the signature of getScrollSize, right now it passes the data over arguments using pointers, best to introduce struct ScrollSize { uint32_t height; uint32_t width; } and return that instead, as an alternative we could return just std::pair<uint32_t, uint32_t> but I believe having named variables avoids the confusion about which side is height and which one is width.

memellis commented 6 months ago

There shouldn't be too much in the way of actually changing it. This will probably require a good amount of changing variables here and there as a lot of the variables are simply int16_t, I think it will just bubble up once you start hacking at it.

Great thanks!

AaronVanGeffen commented 6 months ago

@memellis, I hope you've not put too much energy into this yet.

In the team chat, there's been some discussion about completely reworking how the scrollable widgets are drawn and indexed into, sparked by #2348. Ideally, this would avoid an increase of the Point type's coord size entirely.

memellis commented 6 months ago

@memellis, I hope you've not put too much energy into this yet.

In the team chat, there's been some discussion about completely reworking how the scrollable widgets are drawn and indexed into, sparked by #2348. Ideally, this would avoid an increase of the Point type's coord size entirely.

@AaronVanGeffen, I had an initial play, so happy to stop and work on something else.