Godot-proposals: GDScript should remove the need for most `set_deferred()` calls

[chucklepie]

Describe the problem or limitation you are having in your project:
Background: In my gdscript I called two lines of code on an area2D:
monitored=true
monitorable=true

but after the second line, only monitored was changed, monitorable was not.

I believe this turns out to be due to doing certain things during physics processing can cause issues and set_deferred is required. As far as I know set_deferred is required when setting up scenes, when modifying collision details, as above in my case on changing criteria, plus probably a dozen other things.

The net result was my game had a random bug that took a long time to figure out.

The thing is, why should a programmer know in gdscript that a property they are setting is in the middle of a physics loop and the system may be unstable and they have to provide a workaround?

Surely if this is the case, then on things that can effect physics, the parser should be automatically putting a set_deferred call in?

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
As above, the end programmer should not have to know about the inner workings of Godot and on every line of code think 'do I need to call set_deferred here', the engine should automatically inject deferred calls when required.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
Make it so that set_deferred is not required because it makes no sense to most people.

Is there a reason why this should be core and not an add-on in the asset library?:
Knowing when and when not to call deferred in order to circumvent edge cases on the physics engines should be down the engine and not the end user.

[chucklepie]

If this is not a feasible thing to do, then can somebody who knows about the engine have a section in the documentation that says: set_deferred() is required when doing the following things....

With a nice list than users can read so they can update all the right parts of code to avoid random glitches, bugs and crashes?

[Calinou]

If this is not a feasible thing to do, then can somebody who knows about the engine have a section in the documentation that says: set_deferred() is required when doing the following things....

This is already documented in individual properties/methods (to an extent), but it's probably missing in some places.

Can you make a list of methods/properties where this information is missing? Thanks in advance :slightly_smiling_face:

[KoBeWi]

Some methods throw an error when trying to change their state in a wrong way (e.g. setting disabled on CollisionShape2D during signal emission). Every similar action should show a relevant error, so this is more a bug IMO.

[TheDuriel]

Doing this automatically under the hood would quickly lead to unpredictable behavior.

A defferred call opens a new call stack on the same frame. Which will make it effectively threadsafe.

Instead what should be done, is more clearly label cases in which you reach across threads in an unsafe manner. Both in the Docs, and via runtime warnings.

[chucklepie]

If I vaguely recall, in previous versions 'queue_free()' was 'free()' and had to be wrapped in a yield, so why not the same for others?

However, assuming this is not possiable as above, this is the sum total documentation for monitorable:
"If true, other monitoring areas can detect this area"

I would love to help, but I simply do not know. Maybe a possible solution is parse the code for
ERR_FAIL_COND_MSG.*(Function blocked during in/out signal|set set_deferred)

and do something with the results, assuming the same message is displayed for all occurrences?

[bojidar-bg]

Even though there might be a documentation bug related to not listing all places which require set_deferred/call_deferred, the original proposal was about not needing to use those in the first place.

Now, the main trouble with doing directly calling set_deferred under the hood is that the value would not be changed until the frame is over, meaning the if you do $CollisionShape2D.disabled = true in a physics callback and then immediately read the value of $CollisionShape2D.disabled, it would still be false.

A way to overcome this would be to change the user-facing value immediately, but defer the update of the physics server value. Since the physics server would not accept an immediate change of the value while flushing queries, this does not make some valid usecase impossible. And since the user-facing values are changed and eventually propagate to the physics server, the user's intent is preserved.

Suggested change:

(In the next few code blocks, I'm using [X] to mark a placeholder.)

In order to do that, we can change all the affected properties from this:

void [A]::set_[B]([C] p_value) {
    ERR_FAIL_COND_MSG(PhysicsServer[D]::get_singleton()->is_flushing_queries(), "Use set_deferred");
    [B] = p_value;
    PhysicsServer[D]::get_singleton()->[E](rid, [B]);
}

To something like this:

void [A]::set_[B]([C] p_value) {
    [B] = p_value;
    if(PhysicsServer[D]::get_singleton()->is_flushing_queries()) {
        if (![B]_dirty) {
            call_deferred("_update_[B]");
            [B]_dirty = true;
        }
        return;
    }
    _update_[B]();
}
void [A]::_update_[B]() {
    PhysicsServer[D]::get_singleton()->[E](rid, [B]);
    [B]_dirty = false;
}

Or to this, in order to avoid having an auxiliary method and boolean flag:

void [A]::set_[B]([C] p_value) {
    [B] = p_value;
    if(PhysicsServer[D]::get_singleton()->is_flushing_queries()) {
        PhysicsServer[D]::get_singleton()->call_deferred("[E]", rid, [B]);
    } else {
        PhysicsServer[D]::get_singleton()->[E](rid, [B]);
    }
}

Quick cost/benefit analysis of the change:

• User experience:
  
  
  
  • Surprise value is lowered, as users can now write code without getting caught up by the engine telling them to set_deferred.
  
  
  • Compatibility is retained, meaning that existing set_deferred users do not need to change their code.
  
  
• Maintenance
  
  
  
  • Some corner cases, such as the error popping up when instancing a scene with a disabled CollisionShape2D in a physics callback would get eliminated.
  
  
  • In case the solution is implemented without using an auxiliary method for some property, there is the chance that future contributors would change it so that it makes multiple deferred calls per property set.
  
  
  • Future contributors might forget to use the pattern, but that would affect only new properties. It should be easy to solve them on a case-per-case basis.
  
  
  • Not documenting quirks but eliminating them altogether ensures that the user-facing side of the API and the documentation stay clean.
  
  
• Performance:
  
  
  
  • In the case where users continue using set_deferred manually, this adds a boolean field and a single if. As the if will be predictably false, and as the same condition is checked later in the physics server anyway, it is unlikely to lead to a performance problem. The boolean field would negligibly increase the footprint of the engine, and it can be bitpacked with other boolean flags if necessary.
  
  
  • In the case where users set the property directly while the physics server is flushing queries, this adds either one deferred call or as many deferred call are there are physics server functions to call. As the MessageQueue used to process those deferred calls has limited size, we should probably use an auxiliary function whenever there are multiple physics server methods to be called. At any rate, there is no performance loss from this, as users using set_deferred would have made the same amount of deferred calls.
  
  

[chucklepie]

I like the proposed code changes with the dirty flag, it seems to just
'make it work' and helps remove the need to use deferred, which to me is
to low level a requirement. But I'm not a godot dev 😁

Btw, apologies for the hijack, but is setting disabled true on the
collision box the same as setting monitored and monitorable both to false?

On Mon, 10 Aug 2020, 00:24 Tomek, [e-mail] wrote:

Some methods throw an error when trying to change their state in a wrong
way (e.g. setting disabled on CollisionShape2D during signal emission).
Every similar action should show a relevant error, so this is more a bug
IMO.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot-proposals/issues/1343#issuecomment-671113538,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ADCERRJK2BLCA2E4HSE7SKLR74V2FANCNFSM4PZL7KFA
.

[bojidar-bg]

Btw, apologies for the hijack, but is setting disabled true on the collision box the same as setting monitored and monitorable both to false?

Definitely no. Collision shapes (which have disabled) are not the same thing as areas (which have monitoring/able).

[chucklepie]

I mean is disabling the area2d collision box logically similar to turning
monitoring/monitored off, as in they have the same effect as disabling
detection...

On Mon, 10 Aug 2020, 16:32 Bojidar Marinov, [e-mail]
wrote:

Btw, apologies for the hijack, but is setting disabled true on the
collision box the same as setting monitored and monitorable both to false?

Definitely no. Collision shapes and not the same thing as areas.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot-proposals/issues/1343#issuecomment-671426278,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ADCERROBLBIPUECFRK6ZX4LSAAHHHANCNFSM4PZL7KFA
.