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

Removed _change_notify(property) #45879

Merged
merged 1 commit into from
Feb 10, 2021
Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Feb 10, 2021

  • For inspector refresh, the inspector now detects if a property change by polling a few times per second and then does update the control if so. This process is very cheap.
  • For property list refresh, a new signal (property_list_changed) was added to Object. _change_notify() is replaced by notify_property_list_changed()
  • Changed all objects using the old method to the signal, or just deleted the calls to _change_notify() since they are unnecessary now.

@reduz reduz requested review from bruvzg and JFonS as code owners February 10, 2021 20:22
@Calinou Calinou added this to the 4.0 milestone Feb 10, 2021
@akien-mga akien-mga requested review from pouleyKetchoupp and a team February 10, 2021 21:32
@@ -49,11 +49,7 @@ Variant ArrayPropertyEdit::get_array() const {
}

void ArrayPropertyEdit::_notif_change() {
Copy link
Member

Choose a reason for hiding this comment

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

It's not necessarily to do in this PR, but I guess we could remove _notif_change here and in DictionaryPropertyEdit and use notify_property_list_changed directly in the UndoRedo code which currently uses _notif_change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if these classes are still in use, i have the feeling they are not. Probably in another PR they should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since now there are EditorProperty based editor for arrays and dictionaries.

Comment on lines +2460 to +2455
E->get()->update_property();
E->get()->update_reload_status();
E->get()->update_cache();
Copy link
Member

Choose a reason for hiding this comment

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

If those three are always called together, I guess there could be a helper method to call the three of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think properties call them from within some times or its editors.

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.

Some comments, but looks good overall.

-For inspector refresh, the inspector now detects if a property change by polling a few times per second and then does update the control if so. This process is very cheap.
-For property list refresh, a new signal (property_list_changed) was added to Object. _change_notify() is replaced by notify_property_list_changed()
-Changed all objects using the old method to the signal, or just deleted the calls to _change_notify(<property>) since they are unnecesary now.
@reduz reduz force-pushed the remove-change-notify branch from 49399a8 to 1aa2823 Compare February 10, 2021 22:31
@reduz reduz merged commit ad293a8 into godotengine:master Feb 10, 2021
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.

3 participants