Skip to content

Conversation

@manuq
Copy link
Contributor

@manuq manuq commented Jun 14, 2024

Use the block class in the resources instead of the scene path. This decouples the block data from the block UI. Refactoring the scenes shouldn't break the block data.

@manuq manuq requested review from dylanmccall, wjt and wnbaum June 14, 2024 14:51
@manuq manuq marked this pull request as draft June 14, 2024 15:47

func _populate_block_scenes_by_class():
for _class in ProjectSettings.get_global_class_list():
if not _class.base.ends_with("Block"):
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this being a substring match. Can we instead use ClassDB.is_parent_class https://docs.godotengine.org/en/stable/classes/class_classdb.html#class-classdb-method-is-parent-class to see if the class is a subclass of Block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Checking...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried comparing like this:

		if not ClassDB.is_parent_class(_class.base, &"Block"):
			continue

And with "Block" (no &).. and it didn't work :(

@manuq
Copy link
Contributor Author

manuq commented Jun 14, 2024

@wjt ready for review again, please note the last fixup.

@manuq manuq marked this pull request as ready for review June 14, 2024 16:28
@manuq manuq mentioned this pull request Jun 14, 2024
@wnbaum
Copy link
Contributor

wnbaum commented Jun 14, 2024

@manuq I just went over everything and it looks good to me! Maybe you could do some kind of while loop to see if the parent class is Block, but what you have is good for now. Unfortunately ClassDB only works for built in classes. They should really have a more integrated system with class_name.

@manuq
Copy link
Contributor Author

manuq commented Jun 14, 2024

@manuq I just went over everything and it looks good to me! Maybe you could do some kind of while loop to see if the parent class is Block, but what you have is good for now. Unfortunately ClassDB only works for built in classes. They should really have a more integrated system with class_name.

Yeah. OK thanks, I'll squash the commits and merge.

Use the block class in the resources instead of the scene path. This
decouples the block data from the block UI. Refactoring the scenes
shouldn't break the block data.
@manuq manuq force-pushed the dehardcode-block-path branch from b81c2f7 to c884839 Compare June 14, 2024 20:18
@manuq manuq merged commit 86ff6e7 into main Jun 14, 2024
@manuq manuq deleted the dehardcode-block-path branch June 14, 2024 20:18
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.

4 participants