[Bugfixes] Vehicle tanks ui fix for PR #40399 #40473
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… 'magazine' check and instead use a new 'is_fuelbunker()' and the existing 'is_battery()' checks.
mlangsdorf
suggested changes
May 12, 2020
| @@ -486,6 +486,11 @@ bool vehicle_part::is_battery() const | |||
| return base.is_magazine() && base.ammo_types().count( ammotype( "battery" ) ); | |||
| } | |||
|
|
|||
| bool vehicle_part::is_fuelbunker() const | |||
| { | |||
| return base.is_magazine() && base.ammo_types().count( ammotype( "charcoal" ) ); | |||
There was a problem hiding this comment.
No.
It's bad enough that we're special casing batteries, but this code was written to support arbitrary solid fuel and needs to continue supporting arbitrary solid fuel. Figure something else out.
mlangsdorf
added
<Bugfix>
|
in addition to what mlangsdorf said, if you want to add a commit to my PR you need to make a PR to my branch on my fork. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SUMMARY: Bugfixes "Fix behavior bug with all vehicle parts being listed under 'Tanks' in vehicle interaction UI with PR #40399."
Purpose of change
In PR #40399, this fixes a vehicle display issue found while helping @KorGgenT validate the changes. The vehicle interaction UI displayed all vehicle parts as entries in the 'tanks' category.
Describe the solution
Currently the vehicle_display.cpp goes through all vehicle parts to look for valid categories for engines, tanks, batteries, etc. Currently it determines if it's a 'tank' by checking:
is_tank() || base.is_magazine() || is_reactor()
I first fixed the issue by changing base.is_magazine() to is_battery() but this unhooked the UI for steam engines with fuel hoppers. I then added a new function for is_fuelbunker() to also check for a magazine that is restricted to charcoal fuel type to fix this issue.
Describe alternatives you've considered
I had considered changing the default behavior of vehicle_parts being created with empty magazines but when I made this change it unhooked a lot of other things with the PR.
We could add something like a new tag or similar to designate that the magazine (which is now far more universal than it was before) is a 'fuel' type magazine and then check for this, which would allow us to pull the majority of this logic into JSON instead.
Testing
Validated that these changes don't unhook anything without Korgs changes in PR #40399 as well as that they fix the issue with the changes. Validated with about 10 different types of vehicles, as well as some custom constructed setups which contain multiple types of batteries, steam engine, and diesel engines and tanks.
Additional context
This is a quick fix to address a major bug in PR #40399 for vehicle UI.