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.

Sign up for GitHub

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

Jump to bottom

[Compatibility] Add stub for VisualShaderNodeComment #90797

Merged
merged 1 commit into from
Apr 18, 2024
Merged

[Compatibility] Add stub for VisualShaderNodeComment #90797

merged 1 commit into from
Apr 18, 2024

Conversation

Geometror
Copy link
Member

@Geometror Geometror commented Apr 17, 2024 •
edited by akien-mga
Loading

Fixes part of #90303.

@Geometror Geometror requested review from a team as code owners April 17, 2024 13:06
AThousandShips
AThousandShips approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips 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 otherwise!

scene/resources/visual_shader.h Outdated Show resolved Hide resolved
scene/resources/visual_shader.h Outdated Show resolved Hide resolved
scene/resources/visual_shader.cpp Outdated Show resolved Hide resolved
@Geometror Geometror force-pushed the vsnode-comment-compat branch 3 times, most recently from 9f3d702 to 2e8fd3b Compare April 17, 2024 13:17
@Geometror Geometror mentioned this pull request Apr 17, 2024
Merged
3 tasks
@Geometror Geometror force-pushed the vsnode-comment-compat branch 2 times, most recently from ddcb1ed to 5cab3f3 Compare April 17, 2024 13:22
AThousandShips
AThousandShips approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips 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 for bearing with me

akien-mga
akien-mga reviewed Apr 17, 2024
View reviewed changes
scene/resources/visual_shader.h Outdated Show resolved Hide resolved
scene/register_scene_types.cpp Show resolved Hide resolved
scene/resources/visual_shader.compat.inc Outdated
Comment on lines 33 to 39
void VisualShaderNodeComment::_set_description_bind_compat_88014(const String &p_description) {
}

String VisualShaderNodeComment::_get_description_bind_compat_88014() const {
return "";
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of empty compatibility stubs, WDYT about actually re-exposing the description property, and make it save its value in Object.set_meta?

This would allow users to salvage that information instead of losing it when moving to 4.3.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure whether it's correct to put ADD_PROPERTY in _bind_compatibility_methods() though.

@akien-mga
Copy link
Member

This line should be removed from the 4.2.stable.expected file:

Validate extension JSON: API was removed: classes/VisualShaderNodeComment

@akien-mga akien-mga added this to the 4.3 milestone Apr 17, 2024
@Geometror Geometror requested review from a team as code owners April 17, 2024 14:07
akien-mga
akien-mga reviewed Apr 17, 2024
View reviewed changes
scene/resources/visual_shader.compat.inc Outdated
Comment on lines 33 to 46
void VisualShaderNodeComment::_set_description_bind_compat_88014(const String &p_description) {
set_meta("description", p_description);
}

String VisualShaderNodeComment::_get_description_bind_compat_88014() const {
return get_meta("description", "");
}

void VisualShaderNodeComment::_bind_compatibility_methods() {
ClassDB::bind_compatibility_method(D_METHOD("set_description", "description"), &VisualShaderNodeComment::_set_description_bind_compat_88014);
ClassDB::bind_compatibility_method(D_METHOD("get_description"), &VisualShaderNodeComment::_get_description_bind_compat_88014);

ADD_PROPERTY(PropertyInfo(Variant::STRING, "description"), "set_description", "get_description");
}
Copy link
Member

Choose a reason for hiding this comment

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

Actually now that I think further about it:

  • I think this can be "normal" method bindings in _bind_methods instead of compat bindings. The class itself is already deprecated/for compat so the methods themselves don't need to be too.
  • If so, we can also re-add the description property fully, and document that it doesn't do anything in this new setup, but is used to preserve data authored in earlier Godot versions.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, this means also moving them to visual_shader.cpp (enclosed in #ifndef DISABLE_DEPRECATED) and the .compat.inc file can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, done :)

doc/classes/VisualShaderNodeComment.xml Outdated
Comment on lines 6 to 8
<description>
</description>
Copy link
Member

Choose a reason for hiding this comment

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

We should figure out something to write here too just to keep the completion ratio high in https://godotengine.github.io/doc-status/ :)

akien-mga
akien-mga approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Perfect!

BTW, hello Geometror from the future 👋

Date: Wed, 17 Apr 2024 19:00:41 +0200

@akien-mga
Copy link
Member

akien-mga commented Apr 17, 2024 •
edited
Loading

Seems like these still get raised despite the API being present in the parent class:

Validate extension JSON: API was removed: classes/VisualShaderNodeComment/methods/get_title
Validate extension JSON: API was removed: classes/VisualShaderNodeComment/methods/set_title
Validate extension JSON: API was removed: classes/VisualShaderNodeComment/properties/title

Not sure how we handled this in the past, I think it might be fine to just ignore those in the 4.2-stable.expected file?

Edit: Confirmed, that's what we did in the 4.1->4.2 transition for APIs moved from AnimationPlayer and AnimationTree to the AnimationMixer base class.

@dsnopek
Copy link
Contributor

dsnopek commented Apr 17, 2024

Not sure how we handled this in the past, I think it might be fine to just ignore those in the 4.2-stable.expected file?

Yes, to double confirm: if a method moves up to a base class, it's fine to ignore the message in the .expected file

@akien-mga akien-mga merged commit 2d884ba into godotengine:master Apr 18, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga mentioned this pull request Apr 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips approved these changes

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
breaks compat bug regression topic:gdextension
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
4 participants
@Geometror @akien-mga @dsnopek @AThousandShips