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

Script-class-aware Inspector & related controls. #62413

Merged
merged 1 commit into from
Sep 18, 2022

Conversation

willnationsdev
Copy link
Contributor

@willnationsdev willnationsdev commented Jun 25, 2022

Updates EditorResourcePicker so that global script classes are properly recognized and handled.

Related Issues:

Related PRs:

Sibling PRs:

Considerations:

  • None. Everything should work fine as far as I know. Have tested with regular properties, array properties, make unique, external resources, embedded resources, etc.

@mhilbrunner
Copy link
Member

PR meeting note: we really want this to be in Godot 4.0, but @vnen needs to have a look.

@willnationsdev
Copy link
Contributor Author

@mhilbrunner Sure thing. I'm rebasing all the various PRs atm to make sure they are all up-to-date.

@willnationsdev willnationsdev force-pushed the gdres-inspector branch 2 times, most recently from f8d9058 to 10949c9 Compare August 25, 2022 01:05
@willnationsdev willnationsdev force-pushed the gdres-inspector branch 2 times, most recently from acb6175 to 4ff232a Compare September 16, 2022 04:28
Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

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

I will approve this for now, but the way we are obtaining the class information in EditorData is very hacky. We really need to do a through discussion and cleanup of all the EditorNode structures for 4.1.

@akien-mga akien-mga requested a review from YuriSizov September 16, 2022 11:54
@YuriSizov
Copy link
Contributor

Seems nice otherwise. Definitely shouldn't make things worse, and @akien-mga has reported it working well together with #62411 is his limited testing.

@willnationsdev
Copy link
Contributor Author

I've made the requested adjustments. Should be good to go for merge at this point afaik.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Tested, works well.

The code changes also seem sensible. As reduz pointed out the lack of proper registration of global script classes makes this all a bit awkward but that's a pre-existing design issue that should be solved in a future release.

@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Do you mean to say that I should explicitly use a break inside of an if statement rather than making the any_type_matches variable part of the loop condition, as I have done?

@willnationsdev <...> There was a long reply here explaining what I'd meant, but it seems I was a bit confused and mistaken. Now I'm seeing what you've done originally. 😅

It was fine, execution-wise, but yeah, I think it's better to do this check explicitly in the body. Clearer that way. However, when you've refactored your code after my request, I think you've made a mistake. Your checks are if (!any_type_matches) { break; } now, when they have to be the opposite (and same for paste_valid) and break if there are matches.

@willnationsdev
Copy link
Contributor Author

Oh, crap...lol. Guess I'll need to submit a follow-up PR.

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.

6 participants