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 #11528: Don't auto-build past tunnelbridge ends #11606

Merged
merged 1 commit into from Feb 3, 2024

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Dec 19, 2023

Motivation / Problem

#11528 - autodragging over existing rail/road would start under a bridge, which is not expected behaviour

Description

Keep track of whether we're under a tunnel or bridge when dragging and toggle as appropriate.

Limitations

Feels very "just increase complexity" and there's probably additional edge cases that aren't covered, or are created, by this change.

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 touches english.txt or translations? Check the guidelines
  • 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, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@LordAro LordAro added the preview This PR is receiving preview builds label Dec 19, 2023
@PeterN
Copy link
Member

PeterN commented Dec 19, 2023

Is it possible to make it continue on the other side of a bridge if you drag over it?

@LordAro
Copy link
Member Author

LordAro commented Dec 19, 2023

That's what it does :)

Causes a certain amount of weirdness if you drag over different/multiple bridges though...

@PeterN
Copy link
Member

PeterN commented Dec 19, 2023

Hmm, I dragged over a bridge and it did not continue on the other side.

@Kuhnovic
Copy link
Contributor

This is similar to my PR (draft) from quite a while ago: #9616

One thing to keep in mind is that the "drag and hold ctrl to delete"-behavior will behave different from the "drag to build"-behavior. There are already differences, this will add more differences. Not bad or good per se, but something to consider.

@LordAro
Copy link
Member Author

LordAro commented Dec 30, 2023

The differences as far as the code is concerned is just a toggle of whether the road/rail is built or deconstructed, so i'm not quite sure what the overall problem with the complexity is there.

Though I do agree that as a whole this function is starting to get a bit on the unreadable side

@LordAro
Copy link
Member Author

LordAro commented Dec 30, 2023

On the other hand, your PR is much more "complete" in terms of what it achieves. What would you say still needs doing to it to bring it "up to spec" ?

@Kuhnovic
Copy link
Contributor

Kuhnovic commented Jan 2, 2024

@LordAro The main thing I struggled with was the visualisation of it. I messed around quite a bit to get the highlighting to look somewhat good, but it always felt like a giant hack.

For sure it needs a rebase and a fresh look, the PR is quite old and there's a good change some of the construction logic has changed by now. Unfortunately I have quite limited time available for development ATM...

@TrueBrain TrueBrain added this to the 14.0 milestone Feb 1, 2024
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Might not be perfect, but it is another step in the right direction.

And yeah, all the scoping, it is a bit annoying. I guess we could take a look at "why", as having the if-condition and if-increment not in the if is a bit weird. But, not for this PR!

@TrueBrain TrueBrain merged commit 59f6c19 into OpenTTD:master Feb 3, 2024
20 checks passed
rubidium42 added a commit that referenced this pull request Mar 9, 2024
TrueBrain pushed a commit that referenced this pull request Mar 9, 2024
rubidium42 added a commit that referenced this pull request Mar 9, 2024
michicc pushed a commit to Kuhnovic/OpenTTD that referenced this pull request Mar 9, 2024
Kuhnovic pushed a commit that referenced this pull request Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants