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

Add SkyMaterial base class #101292

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

zhehangd
Copy link
Contributor

@zhehangd zhehangd commented Jan 8, 2025

Implements godotengine/godot-proposals#11518

CI failed at "Check for GDExtension compatibility". I know it is because it changes inheritance, but what am I suppose to do with this error?

@clayjohn
Copy link
Member

clayjohn commented Jan 8, 2025

CI failed at "Check for GDExtension compatibility". I know it is because it changes inheritance, but what am I suppose to do with this error?

Check out our documentation on handling compatibility breakage https://docs.godotengine.org/en/latest/contributing/development/handling_compatibility_breakages.html

@zhehangd
Copy link
Contributor Author

zhehangd commented Jan 9, 2025

CI failed at "Check for GDExtension compatibility". I know it is because it changes inheritance, but what am I suppose to do with this error?

Check out our documentation on handling compatibility breakage https://docs.godotengine.org/en/latest/contributing/development/handling_compatibility_breakages.html

Thanks for the link. But in my case I changed the base class of the three sky material classes to a newly added SkyMaterial class, made Sky accept "SkyMaterial,ShaderMaterial" instead of "ShaderMaterial,PanoramaSkyMaterial,ProceduralSkyMaterial,PhysicalSkyMaterial". It is compatible to the old APIs, but it seems that the validation system detects the change by simply comparsing the strings, it is out of its ability to recognize this change.

Error: Validate extension JSON: Error: Field 'classes/Sky/properties/sky_material': type changed value in new API, from "ShaderMaterial,PanoramaSkyMaterial,ProceduralSkyMaterial,PhysicalSkyMaterial" to "SkyMaterial,ShaderMaterial".

@Calinou
Copy link
Member

Calinou commented Jan 9, 2025

It is compatible to the old APIs, but it seems that the validation system detects the change by simply comparsing the strings, it is out of its ability to recognize this change.

You need to modify https://github.com/godotengine/godot/blob/8c6dbff6d35eb613c83f065c408a49ab02fa7f67/misc/extension_api_validation/4.3-stable.expected to add the validation error to the list below a new title with the PR number (GH-101292 in this case).

@zhehangd zhehangd requested review from a team as code owners January 10, 2025 02:10
@zhehangd
Copy link
Contributor Author

You need to modify https://github.com/godotengine/godot/blob/8c6dbff6d35eb613c83f065c408a49ab02fa7f67/misc/extension_api_validation/4.3-stable.expected to add the validation error to the list below a new title with the PR number (GH-101292 in this case).

Thanks. It seems like validate_extension_api.sh compares the result of extension_api.json difference between 4.x and 4.4 and the accumulated records in extension_api_validation/*.expected from 4.0 to 4.x. One probem is that 4.3 changed the order of the types from "ShaderMaterial,PanoramaSkyMaterial,ProceduralSkyMaterial,PhysicalSkyMaterial" to "PanoramaSkyMaterial,ProceduralSkyMaterial,PhysicalSkyMaterial,ShaderMaterial". If I just add

... "PanoramaSkyMaterial,ProceduralSkyMaterial,PhysicalSkyMaterial,ShaderMaterial" to "SkyMaterial,ShaderMaterial".

to 4.3-stable.expected, 4.3 will pass but 4.0 - 4.2 will fail. So I have to write

... "PanoramaSkyMaterial,ProceduralSkyMaterial,PhysicalSkyMaterial,ShaderMaterial" to "SkyMaterial,ShaderMaterial".
... "ShaderMaterial,PanoramaSkyMaterial,ProceduralSkyMaterial,PhysicalSkyMaterial" to "SkyMaterial,ShaderMaterial".

and it seems to work. Is this an appropriate workaround?

@akien-mga
Copy link
Member

Yes the workaround you found is the one we use currently in such situations. The validation scripts need some work to better deal with successive compatibility changes.

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.

5 participants