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

Allow codegen for UniformSetCacheRD for godot >=4.3 #816

Merged

Conversation

Yarwin
Copy link
Contributor

@Yarwin Yarwin commented Jul 26, 2024

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

What does "reintroduced" mean here? Do we need to explicitly delete it (as you return true)?

@@ -108,6 +108,13 @@ pub fn is_godot_type_deleted(godot_ty: &str) -> bool {
return true;
}

// UniformSetCacheRD has been reintroduced in 4.3
// see: https://github.com/godotengine/godot/commit/5a988456
Copy link
Member

Choose a reason for hiding this comment

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

Could you point to the PR rather than the commit here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 113 to 116
#[cfg(since_api = "4.3")]
if godot_ty == "UniformSetCacheRD" {
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is the #[cfg(since_api = "4.3")] necessary? If it's absent from all versions, we can just take it into the lower list, inside match (including comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Massive brainfart on my side – it should return "false" (this type is NOT deleted since 4.3, it was exposed by accident in 4.1) 😬
Fixed

Copy link
Member

@Bromeon Bromeon Jul 27, 2024

Choose a reason for hiding this comment

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

Edit: misunderstood, makes sense.

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-816

@Bromeon Bromeon force-pushed the feature/reintroduce-uniform-set-cache-rd branch from 361dc61 to 1e04657 Compare July 27, 2024 14:15
@Yarwin Yarwin force-pushed the feature/reintroduce-uniform-set-cache-rd branch 2 times, most recently from 99a0a72 to 549ebe3 Compare July 27, 2024 14:37
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

I wonder if we should still move it down to the match statement, to avoid 2 conditions for the same class. It would also make it very obvious under which circumstances a class is deleted. Same for ThemeDB.

So we would have the following, please copy-paste if possible:

 match godot_ty {
        ... // all the existing stuff
        | "ResourceFormatImporterSaver"
        => true, 

        // Previously loaded lazily in 4.2 it loads at the Scene level. See https://github.com/godotengine/godot/pull/81305.
        | "ThemeDB"
        => cfg!(before_api = "4.2"),

        // Reintroduced in 4.3, see: https://github.com/godotengine/godot/pull/80214.
        | "UniformSetCacheRD"
        => cfg!(before_api = "4.3"),

        _ => false
}

@Yarwin Yarwin force-pushed the feature/reintroduce-uniform-set-cache-rd branch from 549ebe3 to 6badfa2 Compare July 27, 2024 14:51
Comment on lines 130 to 137
=> true,
// Previously loaded lazily; in 4.2 it loads at the Scene level. See: https://github.com/godotengine/godot/pull/81305
| "ThemeDB" => cfg!(before_api = "4.2"),
// reintroduced in 4.3. See: https://github.com/godotengine/godot/pull/80214
| "UniformSetCacheRD" => cfg!(before_api = "4.3"),
_ => false
Copy link
Member

@Bromeon Bromeon Jul 27, 2024

Choose a reason for hiding this comment

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

Sorry for nitpicking, but I explicitly asked to copy-paste because there are several capitalization/period problems 😉

The empty line before the comment is removed by rustfmt? If yes, that's fine, but please refer to my code block suggested above, regarding the comment wording/formatting.

@Yarwin Yarwin force-pushed the feature/reintroduce-uniform-set-cache-rd branch from 6badfa2 to 78344da Compare July 28, 2024 08:18
@Bromeon
Copy link
Member

Bromeon commented Jul 28, 2024

Still a capitalization issue with "reintroduced", but I'll fix this later, takes too much time for both of us otherwise 😉

Thanks a lot for the PR!

@Bromeon Bromeon added this pull request to the merge queue Jul 28, 2024
Merged via the queue into godot-rust:master with commit 66bd5ad Jul 28, 2024
14 checks passed
@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: engine Godot classes (nodes, resources, ...) labels Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: engine Godot classes (nodes, resources, ...) quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants