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

WIP Add reorderstops overlay #889

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

robob27
Copy link
Contributor

@robob27 robob27 commented Nov 8, 2023

Need to do some testing with this and figure out if changing the ids breaks stuff, or if anything else needs to be cleared out like the guide paths. Putting up what I have for now.

trackstop.lua Outdated
end

local function reset_guide_paths(conditions)
for _i, condition in ipairs(conditions) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for unused variables, the Lua convention is just to use an underscore with no other characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah that's it. Was wondering why it still showed greyed out in the editor but hadn't looked into it yet. ty!

trackstop.lua Outdated
Comment on lines 290 to 292
df.global.game.main_interface.recenter_indicator_m.x = item_pos.x
df.global.game.main_interface.recenter_indicator_m.y = item_pos.y
df.global.game.main_interface.recenter_indicator_m.z = item_pos.z
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass another , true to revealInDwarfmodeMap to get this for free

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Siick

trackstop.lua Outdated
@@ -236,7 +260,170 @@ function RollerOverlay:init()
}
end

selected_stop = selected_stop or nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this need to be a global instead of an instance variable in ReorderStopsWindow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess with the way modals work I don't need the other global variable either. Nice. Removed that too.

trackstop.lua Outdated
ReorderStopsWindow.ATTRS {
frame={t=4,l=60,w=45, h=28},
frame_title='Reorder Stops',
view_id='main',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is never referenced, you don't need to give it an explicit view_id

trackstop.lua Outdated

ReorderStopsWindow = defclass(ReorderStopsWindow, widgets.Window)
ReorderStopsWindow.ATTRS {
frame={t=4,l=60,w=45, h=28},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this window contains a list with a variable number of elements, it would be useful to set resizable=true

trackstop.lua Outdated
if item.type == 'stop' then
local stop_index = item.stop_index

-- don't allow moving stops to a different route
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this might be fine, I'll revisit it for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now possible

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit this scares me a little bit, but if @ab9rf gives the ok that we've handled all the potential places where the id is stored, then I'll allow it.

Comment on lines +31 to +34
- `trackstop`: provides 3 new overlays:
- trackstop: allow changing track stop dump direction and friction
- rollers: allow changing roller direction and speed
- reorderstops: reorder stops in hauling routes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this kind of formatting doesn't translate well to the final rendered changelog. each line should be independent. If you start each of them with `trackstop`: then they will get combined into a list similar to your intention here

ReorderStopsOverlay.ATTRS{
default_pos={x=6, y=6},
default_enabled=true,
viewscreens='dwarfmode/Hauling',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this specific enough, or should it disappear on hauling subscreens? We might need to extend Gui.cpp if this is the case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's okay on all of them except them item selection one. Will fix.

}

local SELECT_STOP_HINT = 'Select a stop to move'
local SELECT_ANOTHER_STOP_HINT = 'Select another stop to swap or same to cancel'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right click should also cancel out of a partially completed reorder operation. you can do this by handling keys._MOUSE_R in ReorderStopsWindow:onInput(keys) and returning true if you have handled the right click and don't want the window to close.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@ab9rf
Copy link
Member

ab9rf commented Nov 9, 2023

I have to admit this scares me a little bit, but if @ab9rf gives the ok that we've handled all the potential places where the id is stored, then I'll allow it.

the function where this all takes place in the game is one of the monsters that ghidra cannot decompile (i left it running for six hours and it was unable to do so). clearing the guide path after reordering the stop resolved the odd behavior we were seeing previously. there aren't any other fields involved and since we're just reordering the elements in the vector, pointers to the individual stops are unaffected. stockpile links to hauling stops are by pointer, not by id, so changing the hauling stop id doesn't affect those

the one thing i'm not sure this covers is the vehicle_current_stop (bay12 name) vector in hauling_routest (also bay12 name). basically if you change the order of the stops, you also have to update hauling_route::vehicle_stops to reflect that the index where that vehicle is currently has changed. my brief review of this code suggests that this is not being done, but i might have missed it. basically you need to transform each element of this vector by the remapping involved in whatever reorder you do, so that the minecarts are reported as being at the right place. this is based entirely on bay12 information so you should investigate whether what i'm saying even makes sense before you implement as i could be misinterpreting toady's code entirely

@myk002
Copy link
Member

myk002 commented Nov 9, 2023

also, make sure you test to see what happens if a minecart is en route to a reordered stop, both in the pushing case and the guided case

@robgoodberry
Copy link

Thanks to both of you! Will update and ensure to test these scenarios.

@robob27
Copy link
Contributor Author

robob27 commented Nov 9, 2023

I was wondering why I got a reply for my own notification, replied on my other account 😅 haha

@robob27
Copy link
Contributor Author

robob27 commented Nov 11, 2023

I think the code now handles updating hauling_route::vehicle_stops properly when reordering stops within the same route. I don't currently do anything with hauling_route::vehicle_stops when swapping stops between different routes. I think this works out okay?

If I have this setup:

image

And I swap Stop 1 and Stop 4 (different physical locations):

image

From the same initial setup, if I swap Stop 1 and Stop 5 (same physical locations):

image

Again from that setup, if I swap Stop 2 and Stop 4 (different physical locations):

image

Again from that setup, if I swap Stop 1 and Stop 2 (same route, same result regardless of which stop is selected first):

image

So, if after swapping stops, the cart for that route is not physically at the stop index it was at before the swap, it will be moved there. If the swapped stop is at the same physical location, the cart will be considered in the right spot. If the stops are swapped within the same route, the vehicle index is updated appropriately.

Still working on more playtesting.

@myk002
Copy link
Member

myk002 commented Nov 18, 2023

I have notes on the UX, but they can wait until you have the basic functionality tested

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.

4 participants