ChangeTextureAtlasBuilder into expected Builder conventions
#969
Conversation
| max_size, | ||
| } | ||
| /// Constructs a new texture atlas builder. | ||
| pub fn new() -> Self { |
There was a problem hiding this comment.
#[inline] this. Not sure about the other ones but this follows @cart's general rule about inlining.
| max_size, | ||
| } | ||
| /// Constructs a new texture atlas builder. | ||
| pub fn new() -> Self { |
There was a problem hiding this comment.
I rarely stray from "normal rust" conventions, but I'm not a huge fan of new() aliasing to default(). I prefer "one way" of doing things and new() vs default() creates an arbitrary choice that users need to make (where it doesn't matter one way or the other). I'd prefer it if we just removed new() entirely in favor of default().
There was a problem hiding this comment.
I agree that new can be arbitrary choice for some types, but it isn't always. For types that benefit from being called in a const context, const fn new can provide this for types with private fields, while default and other trait methods cannot be called (yet); however, that doesn't apply to TextureAtlasBuilder.
It can also help discoverability in docs/autocomplete. While I am very comfortable with grokking trait impls on types, it's a skill that takes time for new rust users, and a new can be presented near the top of the page. Thankfully, RA is pretty great at autocompleting default these days though.
There was a problem hiding this comment.
Yeah id definitely make exceptions for const fn new. Docs/autocomplete discoverability is a fair point, but learning to look for default() is a useful skill too 😄
I still personally think that "less is better" here, but if enough people think "new + default everywhere" is the better policy, I'm open to reconsidering. For now lets remove it (because I've been encouraging "default only" elsewhere and have generally followed that pattern). But feel free to form a new() coalition / start a github discussion. If we land on "new() + default()" we can adapt the whole codebase.
There was a problem hiding this comment.
Haha, less sounds good here. I rarely define new for my own types anyway, and the default pattern is consistent throughout bevy. A month ago I was curious what adding a fn default to bevy_utils would look like master...memoryruins:default (then life happened and I forgot about the branch 😅)
There was a problem hiding this comment.
Whoaa thats cool. I'm still sorting out my feelings about it, but it certainly cuts down on boilerplate.
memoryruins
added
C-Code-Quality
|
Thanks for those changes @cart, I was pulling stuff from my spritesheet and missed those doc comments. |
joshuajbouw
force-pushed
the
texture-atlas-builder-better
branch
from
cc772ec
to
7afccb2
Compare
The old API wasn't actual builder format. When making my SpriteSheet and using it as an example, I improved it on my own and decided to implement those changes back to Bevy for the TextureAtlasBuilder.
This is a light API breaking change.