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

Fix: change the bounding boxes of tram catenaries to fit with current sets #8838

Closed
wants to merge 1 commit into from

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 10, 2021

Fixes #8647. Well ... it "mitigates" it.

Motivation / Problem

WARNING: I know nothing about all this; I just applied my knowledge of isometric game engine sorters to OpenTTD .. so be careful :D

Catenaries for tram doesn't appear to have any specs for the bounding box used. This makes it a bit difficult to get them right. I compared the current two I could found: a 8bpp and a 32bpp variant, and checked if they match. I tried to make it look better for those two. This of course is a bullshit approach for the problem, so I am perfectly fine for being called names while this PR is closed :)

Description

Wrong height

Observations: catenaries are set to be TILE_HEIGHT + BB_HEIGHT_UNDER_BRIDGE, but clearly they are larger:
image

This causes catenaries to not been drawn when they should like in the image above.

Similar, this is true for the OpenGFX variant:
image

Solution: TILE_HEIGHT * 2 + BB_HEIGHT_UNDER_BRIDGE. This matches a lot better, but not perfectly between the two sets. There is no correct answer; this one is just better (but still wrong).

Wrong dimension

Because the "front" sprite is considered 16x16, it captures the whole tile. This means sorting goes a bit weird, as these "front" sprites are supposed to be behind the vehicles (the name "front" is really misleading to me, but what-ever). There is here too not a correct answer, just one that might be slightly better: make them 1 by 1 (instead of 16 by 16). This is true for MOST sprites. Some sprites, however, draw both the front and back catenary in the same sprite. This is an impossible situation to solve in terms of sorting.

image
They are off by 1 for the OpenGFX set.

image
They are pretty correct with CZRT.

image
This is zBase; those are even higher, but otherwise also "good enough".

Again, I want to stress, none of this is exact science, and all of this is wrong either way. This PR only appears to make it slightly less wrong in the current most common cases.

The result:

image
Correctly sorted

image
Now breaks; both poles are in the same sprite, so there is no correct way here.

image
Correctly sorted

Limitations

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
Copy link
Member Author

@TrueBrain TrueBrain commented Mar 10, 2021

No, I am taking this back. I knew we had weird sprites in the basesets, but I was just hoping they were used for special places where it was okay to have a pylon on both sides of the rails in a single sprite. But alas ... it is not.

If you draw a pylon on both sides of the rails in a single sprite, it is impossible to sort it correctly. There will always be edge cases where it looks wrong, as .. well .. in isometric world there is no correct answer for that.

Unless someone has a trick up his sleeve, I am just going to close this, and never look back.

@TrueBrain TrueBrain closed this Mar 10, 2021
@TrueBrain TrueBrain deleted the catenary branch Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

1 participant