Open chucklepie opened 4 years ago
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?
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:
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.
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.
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?
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.
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, notifications@github.com 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 .
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).
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, notifications@github.com 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 .
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.