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

Fixed some PreventRoomOverlap issues #260

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Sooslick
Copy link

Fixed some issues and false-negatives in PreventRoomOverlap function:

  1. Added swapping min/max in CalculateRoomExtents function before adjusting size (causing room extents stretch instead of shrink);
  2. Moved CalculateRoomExtents from CreateRoom function to CreateMap to calculate initial extents correctly;
  3. Fixed misleading "replaced room successfully" message for the case where rooms hadn't been replaced at all.

@ChronoQuote
Copy link

+1

I made the same CalculateRoomExtents fix in #210 but I never noticed that the room extents were being calculated before the rooms were rotated into place.

I'm also working on improving the PreventRoomOverlap function.

@Jabka666
Copy link
Contributor

Jabka666 commented Sep 25, 2023

What about CreateRoom functions in Save.bb? You also should call CalculateRoomExtents, shouldn't you?

@ChronoQuote
Copy link

What about CreateRoom functions in Save.bb? You also should call CalculateRoomExtents, shouldn't you?

Room extents are only used by CheckRoomOverlap and PreventRoomOverlap during map generation so they're not needed afterwards.

@Sooslick
Copy link
Author

I made the same CalculateRoomExtents fix

@ChronoQuote Should I remove the same changes as in #210 and change target branch to yours?

Copy link
Contributor

@Jabka666 Jabka666 left a comment

Choose a reason for hiding this comment

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

I think you forgot to put the function after checkpoint rooms placement

EndIf

@ChronoQuote
Copy link

Probably I should add disableoverlapcheck=true for checkpoints in rooms.ini instead

No, there is a difference between the rooms that are hard-coded to be ignored by PreventRoomOverlap and the rooms that have disableoverlapcheck=true. The rooms that are hard-coded to be ignored (checkpoint1, checkpoint2, and start) won't be moved when PreventRoomOverlap gets run on them, but they will still be checked for overlap when PreventRoomOverlap runs on other rooms. On the other hand, any room with disableoverlapcheck=true will neither be moved when PreventRoomOverlap gets run on them nor be checked for overlap when PreventRoomOverlap runs on other rooms. The only rooms with disableoverlapcheck=true are those with geometry above or below them like room049.

So I agree with Jabka that the room extents should be calculated for the checkpoint rooms. I think you can just move the CalculateRoomExtents call on line 7559 a couple lines down below the EndIf.

@ChronoQuote Should I remove the same changes as in #210 and change target branch to yours?

Sure, if you want to do that I would merge it. But maybe that would cause #210 to include too many changes... perhaps it would be better if we moved my two PreventRoomOverlap commits from #210 over to this branch, so that this branch is for room overlap while my branch is for map generation? But I'm not sure how to do that 😅

@Sooslick
Copy link
Author

I agree. I realised checkpoints problem right after sending comment, so I decided to delete it immediately (but too late I guess XD)

perhaps it would be better if we moved my two PreventRoomOverlap commits

I will try to cherry-pick your commits to this branch then

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.

3 participants