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 #9527: Crash when trying to place multitile objects at map edge #9529

Merged
merged 2 commits into from Sep 11, 2021

Conversation

@LordAro
Copy link
Member

@LordAro LordAro commented Aug 31, 2021

Motivation / Problem

#9527

Building objects did not do any bounds checking. Fine for 1x1 objects, less so for others

Also spotted an issue with a previous fix in the same area

Description

Add a check of all the tiles for validity. Not sure if there's a better way.

Also fix the check for zero-size objects, which was missing a bitshift (32 != 0). and bump the grf message up a level (to 0), as grf makers will probably want to see it
Example of issue: 0xF0 & 0x22 === 0x20 === 32 !== 2 Therefore if the lower nibble is 0, it would not be correctly detected as such.

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 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, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
src/object_cmd.cpp Outdated Show resolved Hide resolved
Loading
@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented Sep 11, 2021

The existing zero-object size check is correct. Other than style issues there doesn't appear to be anything being fixed?

Also fix the check for zero-size objects, which was missing a bitshift (32 != 0)

Therefore if the lower nibble is 0, it would not be correctly detected as such.

I don't see how these follow from the existing code. Both nybbles are checked separately, and a bitshift has no effect.

(I've got no objections to this PR, incidentally).

Loading

@LordAro
Copy link
Member Author

@LordAro LordAro commented Sep 11, 2021

Yes, I just became very confused about what it was doing. Commits have since been updated to reflect that it was just a codechange, but PR description had not

Loading

@LordAro LordAro merged commit d4588df into OpenTTD:master Sep 11, 2021
15 checks passed
Loading
@LordAro LordAro deleted the bug9527 branch Sep 11, 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.

None yet

4 participants