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

Unify and streamline connecting to Resource changes #78993

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 3, 2023

Resource's changed signal is one of the most used signals in the codebase. So far there was no standard way to use it, editors and nodes just connected them however they pleased:

  • using connect(CoreStringNames::get_singleton()->changed
  • using connect(SNAME("changed"
  • using connect("changed"

There was also a weird and underused concept of "resource owners". The idea is that you do register_owner(this) and then Resource calls notify_change_to_owners() and you get notified. The problem is that you need to manually register and deregister the resource. Also the callback is a hard-coded "resource_changed" method. On top of that, this mechanism was used in a few 3D-related classes. Some used it instead the changed signal, which made it impossible to listen to changes normally (the resource_changed thing is not exposed). Some classes used both the signal and notify. And then there is ShapeCast3D, which both connected to signal and to the notify, which means that it reacted twice to each change, running the same code in 2 separate methods xd

Another thing is that it's often good practice to use is_connected() before connecting the signal, to avoid errors. This resulted in code like

Callable c= callable_mp()
if (!is_connected(c)) {
    connect(c)
}

or in worse cases the callable was just repeated twice ¯\_( •_•)_/¯

This PR resolves all aforementioned issues by:

  • adding connect_changed() and disconnect_changed() methods to Resource, which always connect with CoreStringNames
  • both methods first check if the signal is (isn't) already connected, also respecting the CONNECT_REFERENCE_COUNTED flag
  • I changed all code to use these methods, which in some cases reduced 4 lines of boilerplate into one
  • this also allowed to remove core_string_names include in some files
  • resource owners are removed and replaced with the usage of connect_changed() and emit_changed()
  • emit_changed() is now public, so it can be called outside the resource (just like in GDScript xdd)

The new methods are not exposed (yet?).

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Looks good but I only looked at the resource changes.

I can't think of a better design.

Edited:

Some documentation changes needed.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 3, 2023

Right, the resource_changed() methods were bound. I guess we could deprecate them 🤔

@KoBeWi KoBeWi requested a review from a team as a code owner July 3, 2023 19:30
@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 3, 2023

Ok, the methods were deprecated. They are just callbacks that should be called by the resource, they were never meant to be used in script. I added some generic description.

@seppoday
Copy link

seppoday commented Jul 5, 2023

Question: Does that PR allow to add "Synchronize Resource Changes" in play mode the same way script and editor changes right now can have effect when game is played? Right now if you change image (resource) you need to reload game. Maybe that PR could be first step to change it?

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 5, 2023

This PR just makes it easier to connect signals. It might help with implementation, but doesn't make anything new possible.
Note that lack of resource synchronization is a bug, as it used to be supported IIRC.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Jul 10, 2023
@YuriSizov YuriSizov requested review from a team and removed request for a team July 14, 2023 19:24
@YuriSizov YuriSizov removed the request for review from a team July 14, 2023 19:24
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

It makes sense on the surface. Needs a rebase though.

Maybe @reduz can tell us something about the idea behind the owners, but to me it looks like some legacy system that can be safely replaced by signals.

@YuriSizov YuriSizov merged commit 1770a2a into godotengine:master Jul 18, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@KoBeWi KoBeWi deleted the making_connections🤝 branch July 18, 2023 10:04
curve->disconnect(CoreStringNames::get_singleton()->changed, callable_mp(this, &CurveEdit::_curve_changed));
curve->disconnect(Curve::SIGNAL_RANGE_CHANGED, callable_mp(this, &CurveEdit::_curve_changed));
curve->disconnect_changed(callable_mp(this, &CurveEdit::_curve_changed));
curve->disconnect_changed(callable_mp(this, &CurveEdit::_curve_changed));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

no .-.

Copy link
Contributor

@YuriSizov YuriSizov Jul 18, 2023

Choose a reason for hiding this comment

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

According to a crude regex, these are the only two

(checked for removed lines with ^-(.*)connect\(("changed"|CoreStringNames|SNAME\("changed"), 270 total, 268 matching this regex)

curve->connect(CoreStringNames::get_singleton()->changed, callable_mp(this, &CurveEdit::_curve_changed));
curve->connect(Curve::SIGNAL_RANGE_CHANGED, callable_mp(this, &CurveEdit::_curve_changed));
curve->connect_changed(callable_mp(this, &CurveEdit::_curve_changed));
curve->connect_changed(callable_mp(this, &CurveEdit::_curve_changed));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too :)

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