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

♻️ rework property type lookup #98

Merged
merged 1 commit into from Sep 1, 2023
Merged

Conversation

bitbrain
Copy link
Owner

@bitbrain bitbrain commented Aug 31, 2023

Prerequisite for:

Description

Complete overhaul of the type system of Pandora by introducing the PandoraPropertyType class. This allows us to make it easier to specify new types, e.g. by adding a new file to res://addons/pandora/model/types/new-type.gd which then internally specifies everything Pandora needs to know, including serialization, property settings and validation!

Highlights

  • removed type settings from UI code
  • remove type parsing responsibility from PandoraProperty and move it over into PandoraPropertyType - instead, it now acts as a "stupid" container
  • clean up some areas, e.g. PandoraProperty now gets its default value from the type, rather than having to pass it in manually
  • updated documentation to reflect the changes

Functionally, this pull request should not change anything critically.

new_property.load_data(property.save_data())
assert_that(new_property.get_default_value()).is_null()
assert_that(new_property.get_default_value()).is_equal("")
Copy link
Owner Author

Choose a reason for hiding this comment

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

These tests now change as it is impossible to create a PandoraProperty that has a valid type but an invalid default value.




func get_default_settings() -> Dictionary:
Copy link
Owner Author

Choose a reason for hiding this comment

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

Settings are now comping from the type directly, rather than from some random UI component.

class_name PandoraPropertyType extends RefCounted


class UndefinedType extends PandoraPropertyType:
Copy link
Owner Author

Choose a reason for hiding this comment

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

This type is useful to avoid null checks but have instead an undefined type.

@bitbrain bitbrain marked this pull request as ready for review August 31, 2023 20:13
if name == "":
return UndefinedType.new()
var ScriptType = load("res://addons/pandora/model/types/" + name + ".gd")
if ScriptType != null and ScriptType.has_source_code():
Copy link
Owner Author

Choose a reason for hiding this comment

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

This explicit check is required since Godot 4.0.x and 4.1.x have different behavior:

  • Godot 4.0.x: load() returns GDScript even if it failed to load
  • Godot 4.1.x: load() returns null

Copy link
Contributor

@eth0net eth0net left a comment

Choose a reason for hiding this comment

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

This all looks great to me!

@bitbrain bitbrain merged commit 98e04ae into godot-4.x Sep 1, 2023
3 checks passed
@bitbrain bitbrain deleted the supported-type-lookup branch September 1, 2023 10:05
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.

None yet

2 participants