Skip to content

Documentation: Add missing required qualifier for various classes #107989

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jojo-1000
Copy link

There is now a standard way to document required virtual methods. I added the required qualifier to the cases I found by a quick code search where an implementation is required (other than EditorImportPlugin, which is in a separate PR #104740 ).

I checked all cases to ensure that they are still up to date.

@Jojo-1000 Jojo-1000 requested a review from a team as a code owner June 25, 2025 20:29
@akien-mga
Copy link
Member

akien-mga commented Jun 25, 2025

Thanks for the contribution.

It doesn't work like this however, qualifiers can't be added manually in the XML. All this is generated by Godot's --doctool, and thus running --doctool on your changes would remove those qualifiers, as those methods aren't actually registered with METHOD_FLAG_VIRTUAL_REQUIRED.

So either their definitions in C++ need to change to include that flag (via GDVIRTUAL*_REQUIRED macros), or they're actually not meant to be flagged as required.

@akien-mga
Copy link
Member

Looking at AudioStream/AudioStreamPlayback, it seems indeed that the methods you identified should actually be defined with _REQUIRED macros. They likely predate the addition of this flavor of macros and were never refactored.

So you would need to do changes like this to match the new qualifiers in the docs (and running --doctool should then reproduce the changes you made manually):

diff --git a/doc/classes/AudioStream.xml b/doc/classes/AudioStream.xml
index 2d40cfa9ab..9a4c2b2af5 100644
--- a/doc/classes/AudioStream.xml
+++ b/doc/classes/AudioStream.xml
@@ -64,7 +64,7 @@
 				Override this method to return [code]true[/code] if this stream has a loop.
 			</description>
 		</method>
-		<method name="_instantiate_playback" qualifiers="virtual const">
+		<method name="_instantiate_playback" qualifiers="virtual required const">
 			<return type="AudioStreamPlayback" />
 			<description>
 				Override this method to customize the returned value of [method instantiate_playback]. Should return a new [AudioStreamPlayback] created when the stream is played (such as by an [AudioStreamPlayer]).
diff --git a/servers/audio/audio_stream.cpp b/servers/audio/audio_stream.cpp
index 440acb72e5..aeac92e3e4 100644
--- a/servers/audio/audio_stream.cpp
+++ b/servers/audio/audio_stream.cpp
@@ -250,11 +250,10 @@ int AudioStreamPlaybackResampled::mix(AudioFrame *p_buffer, float p_rate_scale,
 
 Ref<AudioStreamPlayback> AudioStream::instantiate_playback() {
 	Ref<AudioStreamPlayback> ret;
-	if (GDVIRTUAL_CALL(_instantiate_playback, ret)) {
-		return ret;
-	}
-	ERR_FAIL_V_MSG(Ref<AudioStreamPlayback>(), "Method must be implemented!");
+	GDVIRTUAL_CALL(_instantiate_playback, ret);
+	return ret;
 }
+
 String AudioStream::get_stream_name() const {
 	String ret;
 	GDVIRTUAL_CALL(_get_stream_name, ret);
diff --git a/servers/audio/audio_stream.h b/servers/audio/audio_stream.h
index 57f78dd32b..dcaddd7a79 100644
--- a/servers/audio/audio_stream.h
+++ b/servers/audio/audio_stream.h
@@ -170,7 +170,7 @@ class AudioStream : public Resource {
 protected:
 	static void _bind_methods();
 
-	GDVIRTUAL0RC(Ref<AudioStreamPlayback>, _instantiate_playback)
+	GDVIRTUAL0RC_REQUIRED(Ref<AudioStreamPlayback>, _instantiate_playback)
 	GDVIRTUAL0RC(String, _get_stream_name)
 	GDVIRTUAL0RC(double, _get_length)
 	GDVIRTUAL0RC(bool, _is_monophonic)

As you can see, this has the advantage of simplifying the GDVIRTUAL_CALL code, as the manual error handling is no longer needed (it's done automatically thanks to the required flag).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants