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

Common layer interface #197

Merged
merged 4 commits into from
Mar 26, 2022
Merged

Conversation

aleokdev
Copy link
Contributor

Closes #179.

src/layers/tile/finite.rs Outdated Show resolved Hide resolved
src/layers/tile/infinite.rs Outdated Show resolved Hide resolved
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

The changes look good to me, though I don't quite understand why it closes #179, since the common interface appears to be limited to getting the width/height, which is then non-existent for infinite layers.

In that sense, actually I think a bounds property returning a rectangle might be more useful. It would be rect(0, 0, width, height) for finite layers, but could return the bounding rectangle of the content for infinite layers, providing a straight-forward way of determining which area to process for tiles while rendering.

#179 also mentions the unified storage for finite and infinite tile layers, but I guess that's an idea better split off into its own issue.

@aleokdev
Copy link
Contributor Author

The changes look good to me, though I don't quite understand why it closes #179, since the common interface appears to be limited to getting the width/height, which is then non-existent for infinite layers.

Well, that's really all they have to offer. There's already a get_tile method iirc so what was missing were the w/h properties.

In that sense, actually I think a bounds property returning a rectangle might be more useful. It would be rect(0, 0, width, height) for finite layers, but could return the bounding rectangle of the content for infinite layers, providing a straight-forward way of determining which area to process for tiles while rendering.

Sure, but this would be an even better job for the future tiles iterator. I'll add a bounds function though, it does seem useful.

#179 also mentions the unified storage for finite and infinite tile layers, but I guess that's an idea better split off into its own issue.

Yeah I don't think that's happening at any point, since it'd require another refactor which would completely change the interface again. It's ok as is, I guess.

@aleokdev
Copy link
Contributor Author

I'm merging now and introducing the bounds function in another PR, since right now neither of the finite/infinite tile layers have the function so there's nothing to unify (it's just a new convenience function). I think it deserves its own PR

@aleokdev aleokdev merged commit c499923 into mapeditor:master Mar 26, 2022
@aleokdev aleokdev deleted the common-layer-interface branch March 26, 2022 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add common interface for finite/infinite tile layers
2 participants