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

render.rs: refactor RendererInfo, add RendererFlags bitflags struct #1058

Closed
wants to merge 6 commits into from
Closed

render.rs: refactor RendererInfo, add RendererFlags bitflags struct #1058

wants to merge 6 commits into from

Conversation

LunarLambda
Copy link
Contributor

@LunarLambda LunarLambda commented Dec 23, 2020

implementation of #1054

  • Deprecates the raw fields of RendererInfo
  • Adds getters to RendererInfo

This will allow us to change the internal fields of RendererInfo in the future without forcing a breaking change, if we decide to remove direct field access entirely

  • Adds a bitflags definition for RendererFlags

It is returned by the RendererInfo::flags method

  • Bumps minor version to 35

This represents a (minor) breaking change, since it adds deprecation lints, which may be errors if #[forbid(deprecated)] is used
I did not pull the last version change (0.34.4, from #1057), so there is a small merge conflict in Cargo.toml

@LunarLambda
Copy link
Contributor Author

Apologies for the duplicate commits. Feel free to squash these if/when merging

@Cobrand
Copy link
Member

Cobrand commented Dec 23, 2020

I think there was a bit of a misunderstanding here, I thought you were only going to deprecate max_texture_width and max_texture_height access because there was a special case with them being 0 that the user might not know. The other fields don't need to have accessors for now. If in the future we notice that there is an issue with them like with max_texture_with we will use accessors then, but until then we'll leave them as-is.

That brings me to the second point: max_texture_width() (and the other) should return either Option<u32> or Option<NonZeroU32>, and with a comment for this method to explain what None means exactly here. If we don't do this now, I don't think there is a point in deprecating those fields, unless I have misunderstood something.

@LunarLambda
Copy link
Contributor Author

If we don't add accessors, we have to change the type of the flags field entirely.

The point of deprecating direct access is that we future-proof ourselves for other changes to the internal structure. For example, if we decided to turn the texture_formats field into an array.

@Cobrand
Copy link
Member

Cobrand commented Dec 23, 2020

So I had forgotten about flags and the accessor is fine for that too. texture_formats is debatable since returning a slice is more versatile as you said, but there isn't a reason to deprecate the direct field access either. Future proofing for something we don't even know if it will come one day is not enough to make a breaking change like that however small it is.

That goes the same for name(), we're just giving the .name field which is a &'static str itself, so why the accessor and the deprecating? We'll just force all users using it to change this call for something that adds no value.

If we ever change this field, we'll deprecate it then, but until then .name will stay as-is.

@LunarLambda
Copy link
Contributor Author

I've removed the deprecation attributes and getters for .name and .texture_formats, and changed the max_texture_(width|height) getters to return Option<NonZeroU32>.

@Cobrand
Copy link
Member

Cobrand commented Dec 23, 2020

Thanks, but please use https://doc.rust-lang.org/std/num/struct.NonZeroU32.html#method.new instead of transmute. The end result is the same, but we don't use unsafe where we don't have to.

@LunarLambda LunarLambda closed this by deleting the head repository Aug 31, 2023
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