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 #8131: small bridges also have pillars drawn #8149

Merged
merged 1 commit into from Jun 28, 2020

Conversation

ilayaraja97
Copy link
Contributor

Fixes issue #8131
I have basically removed the code to draw poles below for small bridges as a ground sprite using DrawGroundSpriteAt().

Instead all pillions of bridges will be drawn by DrawBridgePillars()

I am unsure of what all this will break. Please give me some scenarios to test with. I have played around with this and doesn't break the game for me.

Screenshot from 2020-05-15 11-48-08

@LordAro
Copy link
Member

LordAro commented May 15, 2020

Your git config appears to be "broken" - the author of the commit is not linked to your GitHub account. Can you fix this?

@LordAro LordAro requested a review from frosch123 May 15, 2020 08:39
@ilayaraja97 ilayaraja97 force-pushed the fix-8131 branch 2 times, most recently from 3a98c6b to 4e247d9 Compare May 15, 2020 09:03
@LordAro
Copy link
Member

LordAro commented May 15, 2020

I wonder why it was special cased to begin with - there must have been a reason for it at some point, but it dates back further than the history shows -

OpenTTD/tunnelbridge_cmd.c

Lines 1106 to 1116 in efaeb27

if (ti->z + 5 == z ) {
// draw poles below for small bridges
image = b[2];
if (image) {
if (!(_display_opt & DO_TRANS_BUILDINGS)) image = (image & 0x3FFF) | 0x03224000;
DrawGroundSpriteAt(image, x, y, z);
}
} else if (_patches.bridge_pillars) {
// draw pillars below for high bridges
DrawBridgePillars(ti, x, y, z);
}

@ilayaraja97
Copy link
Contributor Author

This is strange. On azure pipelines the check is complete. In github the check is yet to complete.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good, so I'm just going to merge this. Certainly appears that whatever special case made this code necessary originally is no longer necessary. If it turns out it breaks everything, we can just revert it :)

@LordAro LordAro merged commit cf8ccf4 into OpenTTD:master Jun 28, 2020
@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label Jul 27, 2020
@glx22 glx22 added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Nov 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants