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 hex-space draw function to avoid overlaps #2609

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

Sahil-Chhoker
Copy link
Contributor

@Sahil-Chhoker Sahil-Chhoker commented Jan 10, 2025

Summary

This PR fixes the overlapping of hex-tiles.

Before:

Screenshot 2025-01-10 154840 Screenshot 2025-01-10 161539

After:

image image

Grid

Before:

image

After:

image

Bug / Issue

fixes #2438

Implementation

The old code used RegularPolygon to gather hexagon shapes and assemble them using PatchCollection. In the new implementation, all six vertices are calculated using simple math and the origin coordinates, then combined at the end using LineCollection to avoid duplicate edges.

No breaking changes have been made to the API.

Testing

Tested using building a custom model, found no errors.

@EwoutH EwoutH requested a review from quaquel January 10, 2025 12:53
@EwoutH EwoutH added enhancement Release notes label visualisation labels Jan 10, 2025
@quaquel
Copy link
Member

quaquel commented Jan 10, 2025

Thanks for this! I quickly scanned the code, and it made sense. I want to take a closer look, hopefully soon, to see if stuff can be simplified a bit. In the meantime, could you add a before and after of, say, a 20x20 grid next to what you already provided (which looks great, btw)?

@Sahil-Chhoker
Copy link
Contributor Author

Thank you for your response! I’ve added grid images with a 10x10 layout (as the 20x20 grid didn’t look as clean in the screenshot) while keeping the same line-dot pattern for better clarity.

Copy link
Member

@quaquel quaquel left a comment

Choose a reason for hiding this comment

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

Again, thanks a lot for this PR. It's great to see how you solved the problem!

I have left a variety of suggestions for improving the code of this PR. If you have any questions, don't hesitate to ask.

@Sahil-Chhoker Sahil-Chhoker requested a review from quaquel January 13, 2025 06:13
@Sahil-Chhoker Sahil-Chhoker requested a review from quaquel January 13, 2025 10:29
@quaquel
Copy link
Member

quaquel commented Jan 14, 2025

There is still one issue unresolved regarding the alignment of hexes. Make the change shown below:

loc[:, 0] = loc[:, 0] * x_spacing + ((loc[:, 1]-1) % 2) * (x_spacing / 2)

@Sahil-Chhoker
Copy link
Contributor Author

@quaquel I don't know if you were able to see the review I started, in which I clearly attached images of your suggestion not working, so just to make sure I am attaching one image again, your suggestion offsets the placement of agents. I don't know if this is the intended behavior, if it is I shall change the code.
image

@quaquel
Copy link
Member

quaquel commented Jan 14, 2025

I had missed that reply. Sorry. I'll try to take a closer look myself later this week.

@quaquel
Copy link
Member

quaquel commented Jan 18, 2025

Finally had time to look at this again. My suggestion only changed the agent location, but the same correction evidently also needs to be made to the hex centers: so x = col * x_spacing + (row % 2 == 0) * (x_spacing / 2).

However, while figuring this out, I discovered 2 other bugs (which are also present in the current code).

  1. If there are no agents, the modifications to loc given an error because loc = arguments["loc"].astype(float) is empty.
  2. See the figure below. I created a simple grid with 1 central agent in blue and its neighbors in orange.

Unknown

This is most definitely not right and seems to be related to mixing up row and column indexing. I'll try to figure this out asap.

@Sahil-Chhoker
Copy link
Contributor Author

I have added the reviewed changes, including the new empty loc check. And correct me if I'm wrong, the issue with neighbors existed in the previous code and was not introduced by this PR, right?

@quaquel
Copy link
Member

quaquel commented Jan 20, 2025

the issue with neighbors existed in the previous code and was not introduced by this PR, right?

Yes this issue predates your PR. See #2632

@quaquel
Copy link
Member

quaquel commented Jan 20, 2025

have added the reviewed changes, including the new empty loc check. And correct me if I'm wrong, the issue with neighbors existed in the previous code and was not introduced by this PR, right?

I suggest we first merge #2632 so I can fully visually check this PR.

@quaquel
Copy link
Member

quaquel commented Jan 20, 2025

I have merged the bugfix and updated this PR by rebasing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Release notes label visualisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve visual styling of grid drawing
3 participants