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

Feature: Allow flip of train vehicles in depot independently of NewGRF property #6846

Closed
wants to merge 2 commits into from

Conversation

@Palo123
Copy link

Palo123 commented Jul 3, 2018

No description provided.

@frosch123

This comment has been minimized.

Copy link
Member

frosch123 commented Jul 8, 2018

Please explain in more detail the purpose and intentions of your patch.
I have to make various assumptions when reading it.

The diff tells me you do two things:

  • Remove EF_RAIL_FLIPS, ignore what NewGRFs say, and always allow flipping non-articulated vehicles.
  • Change the sprite offsets for flipped vehicles.

For the first part, removal of EF_RAIL_FLIPS:

  • Why? NewGRF should always know better whether it makes sense to flip a vehicle.
  • "Flipping vehicles" is technical debt.
    • For symmetric vehicles it is pointless.
    • For sprites with visible inter-wagon couplings/walk-ways it is worse.
    • For advanced NewGRF it is very hard to deal with, unconditionally enabling it breaks NewGRF which define sprites depending on the surrounding vehicles.
    • It is very easy for NewGRF to implement the identical visual effect by different means.
    • Pending suggestions for expanding vehicle purchase/refit with "views"/"subrefits" renders vehicle flipping obsolete.
  • This part of the patch is a definitive "won't implement"/"rejected".

About the second part, changing sprite offsets:

  • Easy fixable bug: Your diff does not seem to handle GUI scaling.
  • For the actual intentions: I assume this is to position vehicles correctly when NewGRF do not supply extra alignment code for flipped vehicles.
  • Is there any intention for using this?
  • Does changing the offsets actually improve the situation for NewGRF? I suspect that offsets would still be wrong for 4x zoom-in.
  • Are there any existing NewGRF which assume the current offsets? (this is probably hard to answer; but this acts as a tie-breaker, if there are no good reasons for changing offsets)
  • Additional information in context of this:
    • To support long vehicles it has been suggested to add a flag to always position vehicles relative to the vehicle center, i.e. to also change the offsets of shortened non-fliped vehicles.
    • This is to make the sprite position identical to the position variables x/y_pos (NML: other_veh_x/y_offset, NFO: Var 0x62).
    • This implies the "center" of odd-length vehicles would be "off-center" towards the front.
    • This would be a direct conflict with your flipped-offsets. Though vehicles which use x/y_pos would likely disable vehicle flipping in any case.

Summary:

  • Please remove the "ignore EF_RAIL_FLIPS" part. Work with NewGRF, not against them.
  • Elaborate on the considerations/intentions of the "change offsets" part.
@Palo123

This comment has been minimized.

Copy link
Author

Palo123 commented Jul 8, 2018

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:
What I'm trying to fix is flipping of vehicles of shorter lengths. Flipping only works for vehicles of default length (8). For shorter trains it has wrong offset and there are large visible glitches. This is only reproducible with NewGRFs introducing new trains, because default TTD vehicles have default length. This bug is normally not visible, because it's hidden by "EF_RAIL_FLIPS" and let for NewGRF authors.

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;

This comment has been minimized.

Copy link
@frosch123

frosch123 Jul 22, 2018

Member

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.

This comment has been minimized.

Copy link
@frosch123

frosch123 Jul 22, 2018

Member

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;

This comment has been minimized.

Copy link
@frosch123

frosch123 Jul 22, 2018

Member

I suggest to keep this check at this position, and to not move it after IsStoppedInDepot.

@frosch123

This comment has been minimized.

Copy link
Member

frosch123 commented Jul 22, 2018

About the 4x zoom:
In higher zoom levels small errors in alignment are a lot more noticeable than in 1x zoom.
For example in case of road vehicles: It is near impossible to align road vehicles so that they look good for both "drive on left" and "drive on road".

In this case for trains:
I gave it a second thought. Since the patch moves the vehicles only along the track and not across the track, I think this does not affect the 4x alignment. So, should be fine :)

@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Nov 1, 2018

Ping @Palo123, any progress on this?

@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Jan 5, 2019

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!

@andythenorth andythenorth added the stale label Jan 6, 2019
@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Jan 9, 2019

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.