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

Auto-align in both directions #1780

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ralfgerlich
Copy link

Current auto alignment only aligns to the right, but not to the left (as lamented in #486). This PR introduces bi-directional auto-alignment for the X-axis.

@ralfgerlich
Copy link
Author

Just realised that the underlying assumption is wrong according to which only walls adjoining the same sector as the initial wall have to be aligned. It's a small fix, but I don't know whether I'll have time before next week.

@ralfgerlich ralfgerlich marked this pull request as draft February 9, 2025 20:06
@ralfgerlich
Copy link
Author

I have extended the pull-request to jump across two-sided lines. For that I introduced a few helper methods in MapSide.

There is a small open issue in MapSide::nextInSector and MapSide::prevInSector: Any sector should have at least two sides, so these functions should be able to return something other than nullptr. However, can we be certain that the map we're working with actually fulfills this condition? So far, I err on the side of caution by returning a nullptr in that case and checking for nullptr before application, so it should work regardless even with faulty maps, but I'd rather report a warning here.

Should I use log::warning, or should there be checks when loading the map? What's the right(TM) way to handle this according to SLADE standards?

@ralfgerlich ralfgerlich marked this pull request as ready for review February 9, 2025 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant