Added confirmation dialog to vehicle/wagon deletion, but only for vehicles that currently have cargo aboard#3323
Conversation
- If the user attempts to delete a wagon from a train which has cargo aboard in any wagon, ordinarily this would delete all cargo from the train due to how the vehicle is removed and replaced from the game board - With this commit, players are prompted to confirm their intention to delete the wagon by informing them the action will clear ALL cargo from the train, not just what's aboard the wagon being deleted - If no cargo exists on the train, no confirmation dialog is given, and the behavior is as before - If the entire train is deleted, it behaves as before with no confirmation given - ONLY deleting a single wagon from a train which has cargo somewhere onboard generates a dialog - To implement this, a global, temporary state-tracking struct was added to coexist alongside GameState and is cleared on map/scenario load. This allows for passing state out of GameCommands and can eventually be replaced with something better when the GameCommand system is overhauled - Small utility functions (Vehicle::hasAnyCargo()) added for implementation - Strings in en-GB.yml and header files updated to reflect changes.
AaronVanGeffen
left a comment
There was a problem hiding this comment.
I think this is a nice QoL feature, but the implementation needs a bit of work. To be honest, I think it's more complex than it needs to be.
You've added a nice helper function, VehicleHead::hasAnyCargo. I think this can and should be called directly from the Vehicle window, before even invoking any game command. Only after the confirmation prompt has been shown (or indeed wasn't necessary) should the VehicleSell command be invoked.
By leaving this all in the UI, there is no need for modifications to any game commands, nor any need for the TempState you've introduced.
Does this make sense?
- removed TempState implementation - reverted changes to VehicleSell command - migrated check entirely to Ui/Vehicle.cpp - extracted new functionality to separate method (checkDeletion) to declutter the scrollDragEnd switch block
Loud and clear--and you're right, it's a lot simpler to just do it in the UI. This actually is how it started, but I was hopelessly distracted by the lack of ability to read a return code from the GameCommand--it didn't occur to me that it would be seen as acceptable to do all the casting and checking for Regardless of any excuse on my part, I've simplified it down to be concentrated in VehicleSell.cpp, and kept the |
luciditee
left a comment
There was a problem hiding this comment.
Reviewed; see most recent comment.
9e4587a to
24dfd68
Compare
LeftofZen
left a comment
There was a problem hiding this comment.
- The naming of some of the strings and methods is off. You're selling a vehicle component, not deleting a vehicle.
- The problem still exists for vehicles with reconfigurable cargo - you can change the cargo configuration and it deletes the cargo with no warning. I would probably add that check in this PR too.
Hmm, you make a good point, but at the same time... it is a trash can you're dragging the component to. I can see the confusion. 'Selling' is probably better, though, yeah.
That's a different check that would require a different prompt, right? Agreed it would be good to add it there, too, though. |
Good point, you'd need to add some extra strings for the message in that context. I still think it can be done in this PR though as extra strings + the code change for the additional check should be minimal |
Co-authored-by: Benjamin Sutas <bigbadbennybrother@gmail.com>
Co-authored-by: Benjamin Sutas <bigbadbennybrother@gmail.com>
- Users now receive a prompt when asked to refit a vehicle in the same fashion as when they alter a component - Updated string handles in StringIds.h and added necessary strings into en-GB.yml
|
Had a little trouble there with clang-format showing whitespace that the IDE wasn't showing, but it should be good to go now. Players now get a warning when refitting a vehicle that has cargo already aboard. Any other recommendations?
|
Co-authored-by: Benjamin Sutas <bigbadbennybrother@gmail.com>
A few nights ago I lost a particularly grueling scenario playthrough due to not realizing that deleting a wagon off a train actually clears all cargo values from the vehicle. This inspired me to add a confirmation dialog to the specific case of deleting part of a vehicle that has cargo already loaded onboard.
There is not currently a way to directly pass state from a GameCommand to the calling context. It would theoretically be possible to do via GameState, but I didn't feel comfortable modifying the GameState struct since it appears that the size of the GameState struct matters enough for there to be an assertion about it. For this reason I added a new struct--TempState--which can be used for ephemeral values that should not replicate over the network or survive a save/load operation.This allows us to pass state out of a GameCommand context. It can be removed or replaced with something better later.It currently automatically clears itself anytime a scenario or savegame is loaded.owner == CompanyManager::getControllingId().hasAnyCargo(), was added toVehicleHeadto simplify the predicate in VehicleSell.cpp.I've attached a scenario containing a test case set up so that you can try deleting a wagon from the train, which has cargo aboard, and notice the confirmation dialog. Once you delete a wagon, the cargo values are reset to 0 and if you attempt to delete another wagon, it will do so without confirming because the train has no cargo aboard. I have attached it to this PR (zipped because GitHub doesn't understand the file format): SellConfirmTest.zip
I also uploaded a video to the OpenLoco Discord showing this feature in action (this was too big for me to embed here on GitHub): https://cdn.discordapp.com/attachments/689445672394555507/1426766722118979644/WagonDeleteTest_1.mp4?ex=68ec6b8e&is=68eb1a0e&hm=bc59c97378e5c3c62877e7203310998953bd300dc3617e0228025750c620b5f9& (if you are uncomfortable clicking the link, you can find it in #openloco-talk on the Discord).
EDIT: Omitted a word by mistake.