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

Implement std::fmt::Debug for Map manually #305

Merged
merged 1 commit into from
Aug 4, 2024

Conversation

erayerdin
Copy link
Contributor

@erayerdin erayerdin commented Aug 3, 2024

Closes #138
Self-explanatory

@erayerdin erayerdin marked this pull request as draft August 3, 2024 20:18
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.

Sadly this means that we'll need to update Debug every time a member changes in Map, but it's better than what we had before IMO. Thanks!

@aleokdev
Copy link
Contributor

aleokdev commented Aug 3, 2024

Didn't see this was a draft -- Are you planning to add anything else?

Comment on lines +79 to +80
.field("tilesets", &format!("{} tilesets", self.tilesets.len()))
.field("layers", &format!("{} layers", self.layers.len()))
Copy link
Contributor Author

@erayerdin erayerdin Aug 3, 2024

Choose a reason for hiding this comment

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

Instead of debug-printing all tilesets and layers, we only debug-print the length of self.tilesets and self.layers.

I don't know if this is a viable approach as it calls len and allocates String with format!, which would be not as performant as desired. On the other hand, we're talking about Debug here, which should/must be only relied upon debug builds, not release builds (only logs maybe).

@erayerdin
Copy link
Contributor Author

Didn't see this was a draft -- Are you planning to add anything else?

I was checking TileLayer, FiniteTileLayer, InfiniteTileLayer and map_wrapper! macro to see how I could implement Debug for these but it seems they are already done.

@erayerdin
Copy link
Contributor Author

erayerdin commented Aug 3, 2024

@aleokdev This seems like it would not require rewriting the whole implementation for Map (this one specifically). This, however, would add extra to compile time as derivative heavily relies on macros.

If you find this viable, I could go down this path. :)

@aleokdev
Copy link
Contributor

aleokdev commented Aug 3, 2024

It's ok, we'll do it manually - Same thing we have done for Error (we could've used thiserror but chose not to)

@erayerdin erayerdin marked this pull request as ready for review August 3, 2024 20:37
@erayerdin
Copy link
Contributor Author

Well, I'm going to check TileLayer stuff after I wake up. Thank you for your time. :)

@erayerdin
Copy link
Contributor Author

@aleokdev

I've checked the TileLayer. Apparently, it is an enum with FiniteTileLayer and InfiniteTileLayer, both of which implement Debug manually (well, at least their inner data FiniteTileLayerData and InfiniteTileLayerData do), and they do not seem to include Vecs to their debug-print.

impl std::fmt::Debug for FiniteTileLayerData {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("FiniteTileLayerData")
.field("width", &self.width)
.field("height", &self.height)
.finish()
}
}

impl std::fmt::Debug for InfiniteTileLayerData {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("InfiniteTileLayerData").finish()
}
}

So, I think it's good to go. Feel free to merge. 🍺

@aleokdev
Copy link
Contributor

aleokdev commented Aug 4, 2024

Great, thank you!

@aleokdev aleokdev changed the title [WIP] Implement std::fmt::Debug for Map and TileLayer manually Implement std::fmt::Debug for Map and TileLayer manually Aug 4, 2024
@aleokdev aleokdev changed the title Implement std::fmt::Debug for Map and TileLayer manually Implement std::fmt::Debug for Map manually Aug 4, 2024
@aleokdev aleokdev merged commit ab62d91 into mapeditor:current Aug 4, 2024
4 checks passed
@erayerdin erayerdin deleted the feat/better-debug branch August 4, 2024 17:29
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.

Implement a proper Debug for Map / TileLayer
2 participants