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
Feature: Allow flip of train vehicles in depot independently of NewGRF property #6846
Conversation
Please explain in more detail the purpose and intentions of your patch. The diff tells me you do two things:
For the first part, removal of EF_RAIL_FLIPS:
About the second part, changing sprite offsets:
Summary:
|
Vehicle position offset has a story that is traceable in the tracker. Long ago, position of vehicle wasn't in the center, which brought complicated bugs and glitches. Because of this, there was introduced "EF_RAIL_FLIPS" to forbid such bugs when flipping train in depot by default. After some time, vehicle position was moved to center, but this variable wasn't reviewed, nor the flipping. I see the closure of this story is not a priority because it's convenient to keep it for other reasons, so I'll remove it from patch. The offset part: Basic problem is, when NewGRF author wants his (short) train to flip, he would need to define special sprites for vehicles of shorter lengths due to wrong offset in game or make other workaround. Due to this, we have flipping feature disabled in almost every NewGRFs even when there is no reason for it. Can you please give an explanation on what is wrong with 4x zoom? If you are referencing zoom-in and zoom-out with mouse wheel, there are no problems, this patch doesn't change anything from that code. It only change basic vehicle values that are used later (not the actual drawing code) and then the GUI that is not influenced by zoom. |
@@ -444,6 +444,9 @@ int Train::GetDisplayImageWidth(Point *offset) const | |||
|
|||
if (offset != NULL) { | |||
offset->x = ScaleGUITrad(reference_width) / 2; | |||
if (HasBit(this->flags, VRF_REVERSE_DIRECTION)) { | |||
offset->x -= (VEHICLE_LENGTH - this->gcache.cached_veh_length) * reference_width / VEHICLE_LENGTH; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line only works when GUI zoom is set to 1x.
Make sure that you use
offset->x = ScaleGUITrad(...);
and do all stuff related to cached_veh_length within the "...", or first compute the unscaled position and then call ScaleGUITrad once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also please test the behaviour if you switch to a language with right-to-left text, like arabic or hebrew.
IIRC the trains switch direction in depot for those languages, and this may affect the offsets here.
@@ -1904,13 +1909,13 @@ CommandCost CmdReverseTrainDirection(TileIndex tile, DoCommandFlag flags, uint32 | |||
if (v->IsMultiheaded() || HasBit(EngInfo(v->engine_type)->callback_mask, CBM_VEHICLE_ARTIC_ENGINE)) { | |||
return_cmd_error(STR_ERROR_CAN_T_REVERSE_DIRECTION_RAIL_VEHICLE_MULTIPLE_UNITS); | |||
} | |||
if (!HasBit(EngInfo(v->engine_type)->misc_flags, EF_RAIL_FLIPS)) return CMD_ERROR; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to keep this check at this position, and to not move it after IsStoppedInDepot.
About the 4x zoom: In this case for trains: |
Ping @Palo123, any progress on this? |
We recently switched from Jenkins as CI to Azure Pipelines as CI. This means you need to rebase before this Pull Request will pass its checks. Sorry for the troubles! |
Thanks for this. There's been no activity on this for some time, and as it stands, it doesn't look likely that it will go any further. I'm closing it as we try to keep the open PR count low for OpenTTD, it helps us focus on things that are important and fun. Feel free to discuss in irc or request re-opening if you disagree. Thanks for contributing! |
No description provided.