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.

Sign up for GitHub

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

Jump to bottom

Fix some tram catenary sorting issues #8842

Closed
wants to merge 3 commits into from
Closed

Fix some tram catenary sorting issues #8842

wants to merge 3 commits into from
+2 −2

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 10, 2021 •
edited
Loading

Fixes #8647
Breaks OpenGFX.

Motivation / Problem

Attempt #2, after @glx22 explained a few things to me about catenaries :D Tnx for that!

Basically, there appears to be some weirdness around tram catenaries. So far I collected the following list:

  • In the code, front is the back sprite, and back is the front sprite.

  • OpenGFX has many front sprites that do not redraw the front pylon, or the east/west. This currently "works", because front/back sprites have the same order. After this fix, you get glitches like this:
    image

  • Front/back sprites are ordered exactly the same, so a vehicle is either before both or behind both. This currently works out (most of the time) as most vehicles are lower than the start of the pylons in most sets.

  • The pylons in most sets are visibly higher than the bounding box. This causes glitches, like:
    image
    image

  • zbase is in a league of its own. No clue what is going on there, but it aint right.

Description

After a bit of discussion on IRC, and as we all know nobody really understand NewGRF (besides @frosch123 ; so he might be laughing reading this, as I got this all wrong), I come to this solution with the following arguments:

  • Height is a bit weird, but increasing it makes it less often wrong. Ideally a set can indicate how high pylons are, but this is currently not possible. Maybe it should be part of the spec that it should be N pixels, but that is a different discussion.
  • Back sprites should be sorted behind the vehicle. Front sprites before vehicles. It is up to the NewGRF author to make this look pretty. So the back sprite is now sorted as if it is on the north corner, where the front sprite is not sorted as if it is on the south corner. With this, bounding boxes look a bit crap.

Funny enough, elrail has a completely different implementation for catenaries, where pylons and wires are separated. That for sure feels a bit more natural. Now NewGRF authors need to understand tram catenaries a lot more where things are drawn; OpenGFX shows this is not trivial.

Either way, with this PR, OpenGFX really needs fixing, as it visually glitches a lot more.

The result, a bit better looking catenaries:
image
image

zBase still looks weird though:
image
Seems front pylons are wrong or something. Dunno exactly, but there are double pylons everywhere (already the case before this PR too).

Limitations

  • I might be completely wrong with this fix
  • OpenGFX has poor tram sprites (how I and @glx22 understand it) and requires fixing!
  • Height is an estimate, and can still be wrong. It is just less likely, and most vehicles are smaller than (TILE_HEIGHT + BB_HEIGHT_UNDER_BRIDGE) / 2, as otherwise they would look crap under bridges. This is also the minimum height needed to be sorted wrong with the new approach.
  • I did not fix the back/front mixup in the code, as it is just really confusing to me if I am reading it wrong, or that the code is wrong, or the specs, or the examples, or ....

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain
Fix: front tram pylons are sometimes behind a vehicle
8ab5a2e 
Pylons are assumed to be TILE_HEIGHT + BB_HEIGHT_UNDER_BRIDGE
in height, but in all current sets they are all a lot higher. In
result, depending on where the vehicle is, this can result in poor
sorting.

This commit increases the height, so sorting becomes a bit better,
but it is still an estimation of the height. Sets do differ from
this height, but all (current) are at least higher.
@TrueBrain
Fix: sort tram cateneries as: back -> vehicle -> front
9d7a15d 
By making the bounding box of the back (called "front" in the code,
to add to the confusion) smaller, it is always sorted behind the
vehicle on the track. The front (called "back") is always sorted
in front of the vehicle.

This solves many sorting issues; although most sets have the
vehicle height lower than the start of the pylon, hiding this
problem. Any vehicle that is higher, was sorting wrongly.
@TrueBrain
Codechange: remove dangling space in the middle of a statement
c034057 
Sometimes my OCD triggers.
@TrueBrain TrueBrain mentioned this pull request Mar 10, 2021
Merged
@TrueBrain TrueBrain deleted the catenary-2 branch October 31, 2021 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Road and Tramway Catenary Drawn Incorrectly / Unsorted
1 participant
@TrueBrain