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

Map::layers() doesn't really list ALL layers - added a comment about it #306

Merged
merged 1 commit into from
Aug 18, 2024

Conversation

HughHoyland
Copy link
Contributor

@HughHoyland HughHoyland commented Aug 18, 2024

Took me a while to realize when I tried to load one especially large map.

I guess layers API needs a bit of improvement, but let's start with a comment.

@HughHoyland
Copy link
Contributor Author

I see an even bigger problem.

While Layer<'a> is supposed to live as long as the Map, getting Layer out of a LayerType::Group(GroupLayer) only gives one a Layer<'a> where 'a is the lifetime of the local copy of GroupLayer.

Which means it cannot be passed outside of a function.

It is probably safe to cast... in an unsafe block.

@aleokdev
Copy link
Contributor

I see an even bigger problem.

While Layer<'a> is supposed to live as long as the Map, getting Layer out of a LayerType::Group(GroupLayer) only gives one a Layer<'a> where 'a is the lifetime of the local copy of GroupLayer.

This is a bug. I see the problem: get_layer returns Layer<'self> instead of Layer<'map> + 'map. Has happened elsewhere before. Sorry about that, I'll submit a fix ASAP.

Copy link
Contributor

@aleokdev aleokdev left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!
Please also add your changes to the CHANGELOG.md file under a new section:

## [Unreleased]
### Changed
- Improved documentation on `Map::layers` and `Map::get_layer`. (#306)

@HughHoyland
Copy link
Contributor Author

Added the changelog.

Copy link
Contributor

@aleokdev aleokdev left a comment

Choose a reason for hiding this comment

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

Thank you!

@aleokdev aleokdev merged commit 8c4f5b4 into mapeditor:current Aug 18, 2024
4 checks passed
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.

2 participants