Skip to content

Make required virtual methods abstract #826

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

CedNaru
Copy link
Member

@CedNaru CedNaru commented May 3, 2025

So far we have ignored the "isRequired" field for methods.
This property is always used alongside virtual methods and indicate that if you inherit its class with a script, it's mandatory to override this virtual methods (while others are just optional).
In such case, I made the method (and the class) abstract instead of giving it a dummy implementation.

A side effect is that several classes in the Godot API inherit those now "abstract" classes, so I had to create dummy implementation for those abstract methods.

image

@CedNaru CedNaru requested review from chippmann and piiertho May 3, 2025 01:16
@CedNaru CedNaru force-pushed the feature/generate-abstract-api-classes branch 2 times, most recently from 440cf5e to f3f92c3 Compare May 3, 2025 02:00
@CedNaru CedNaru force-pushed the feature/generate-abstract-api-classes branch from f3f92c3 to 979ab47 Compare May 3, 2025 09:22
@CedNaru CedNaru requested a review from Copilot May 26, 2025 09:57
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the handling of virtual methods marked as isRequired by making their declaring classes and methods abstract instead of providing dummy implementations, and then adding JVM stubs in concrete subclasses. It also adjusts the code generator to respect abstract/override modifiers and updates engine type registrations to use null constructors for abstract types.

  • Mark required virtual methods and their classes as abstract rather than throwing NotImplementedError in base.
  • Inject override stubs (throwing NotImplementedError) into concrete subclasses for abstract methods.
  • Enhance the code-generation phase (EnrichedMethod, EnrichedClass, RegistrationRule, MethodRule, ClassRule, ApiRule) to emit proper modifiers and registration entries.

Reviewed Changes

Copilot reviewed 150 out of 150 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
kt/godot-library/.../AudioEffect*.kt, AtlasTexture.kt, ArrayMesh.kt, AnimatedTexture.kt Made base class methods abstract and added JVM stub overrides.
kt/godot-library/.../AnimationNode*.kt, AnimationMixer.kt, AStar*.kt Converted virtual methods to abstract and provided stubs.
kt/godot-library/.../RegisterEngineTypes.kt Changed registrations to null constructor for abstract types.
kt/api-generator/.../EnrichedMethod.kt, EnrichedClass.kt Introduced Modifier enum for methods and flagged abstract.
kt/api-generator/.../RegistrationRule.kt Updated registration logic to emit null for abstract classes.
kt/api-generator/.../MethodRule.kt Emit abstract, override or stub implementations properly.
kt/api-generator/.../ClassRule.kt Adds generation of override tasks for parent abstract methods.
kt/api-generator/.../ApiRule.kt Filter out singleton-extended classes earlier.
Comments suppressed due to low confidence (1)

kt/api-generator/src/main/kotlin/godot/codegen/models/enriched/EnrichedMethod.kt:13

  • The parameter name override conflicts with Kotlin's override keyword and will not compile. Rename this parameter (e.g. isOverrideFlag) to avoid using a reserved keyword.
class EnrichedMethod(private val model: Method, override: Boolean = false) : CallableGeneratorTrait, DocumentedGenerationTrait {

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