Possible bad behavior in spatial controls. [Fixed and didn't get my way]

Just transferred my autopilot class to a control and noticed something slightly weird.



Whether the control is enabled or not, the control’s update method is invoked. I don’t think it should be the case. That’s easily fixed by checking if it’s enabled in the control’s update loop, but I think the spatial should check before invoking it.



That’s in Spatial.java (line 534 - 542):

[java]

private void runControlUpdate(float tpf) {

if (controls.isEmpty()) {

return;

}



for (Control c : controls.getArray()) {

c.update(tpf);

}

}

[/java]



I would change it to:

[java]

private void runControlUpdate(float tpf) {

if (controls.isEmpty()) {

return;

}



for (Control c : controls.getArray()) {

if (c.isEnabled()) {

c.update(tpf);

}

}

}

[/java]



Thoughts?

3 Likes

stumbled into this myself while building a autopilot control :stuck_out_tongue:

for the moment I do my own checking inside the update method. I think jME should do it like your propose :slight_smile:

as i seen in documentation. isEnabled actually should be checked in Control update



https://wiki.jmonkeyengine.org/legacy/doku.php/jme3:advanced:custom_controls



but yes, it should be automaticly. so i agree



edit: and +1 :slight_smile: ofc

My proposed behavior is the same as pre/main/post viewports. If they’re not enabled, they’re not invoked.



But, there might be a logical reason why it’s invoked no matter what.

1 Like
@oxplay2 said:
as i seen in documentation. isEnabled actually should be checked in Control update


Yeah. But the code snippet right beneath says:

[java]
@Override
public void update(float tpf) {
if (enabled && spatial != null) {
// ...
// Write custom code to control the spatial here!
}
}
[/java]

So... lol
1 Like

i think everyone can do mistake, even devs :wink:

if they gived method “isEnabled” to override, then it should be used, so it should be used in Spatial.java



i created my superclass control with simpleUpdate abstract method, where it is invoked when control is enabled.

But if this would be added into jme then i have one invocation less, so its good fix/idea :slight_smile:



edit: just to be sure, i agree with you.

It was probably just an oversight, but I won’t commit anything w/o the say so. :wink:

btw: runControlRender should be changed too, if we have right.

1 Like

Yes, you’re right. Fixed it. Waiting for instruction… :wink:

yh its something i just got used to, adding it to all my controls :stuck_out_tongue:

Hi,



i completely disagree.



The controls themselves should handle if they’re enabled or disabled - if they wont be updated anymore, someone would have to enable disable them from outside.



I even suggest to remove the isEnabled from the interface…



Regards



.

@snareoj2 said:
Hi,

i completely disagree.

The controls themselves should handle if they're enabled or disabled - if they wont be updated anymore, someone would have to enable disable them from outside.

I even suggest to remove the isEnabled from the interface...

Regards

.


You could always have the option to not use enabled for whatever "disabling" you want to do and just do your own tracking. But if a control is setEnabled(false), I think it should not be called... just like every other case in the engine that has an enabled flag.

Otherwise, yeah, just get rid of setEnabled()/isEnabled()/etc because if the engine doesn't use them then there is no reason to have them.

However, usually it is strange for a control to self-disable or self-enable. Often it is an app state or another control doing this. I think "enabled" has utility and should behave consistent with the rest of the engine.

I think that if it’s disabled, then the control shouldn’t be called. How can a control do anything if it has no power?

@shirkit said:
I think that if it's disabled, then the control shouldn't be called. How can a control do anything if it has no power?

It might still have to check something on the update loop but still yield no effect. Remove it from the spatial to have it not called at all.
@normen said:
It might still have to check something on the update loop but still yield no effect.


It's a control, if it's disabled it shouldn't check the update loop. I think it should behave the same as an AppState or a Viewport.
1 Like
@madjack said:
It's a control, if it's disabled it shouldn't check the update loop. I think it should behave the same as an AppState or a Viewport.

Well I disagree. At any instance its an interface so no way to add functionality anyway and AbstractControl does it the way you indicate.

Well, I don’t agree nor disagree, I see both options as correct.

@normen said:
Well I disagree. At any instance its an interface so no way to add functionality anyway and AbstractControl does it the way you indicate.


Then please explain the existence of setEnabled and isEnabled if they're not taken into account? What's their use?
1 Like

As I said, they enable or disable the effect of the control on the spatial. The control might still need to plot a path or something. Or you want to have setEnabled() thread safe… Then you’d also have to detach or w/e you have to do on the update loop. What setEnabled does is not really defined and as said it cannot be as its just an Interface.

Then I don’t understand the design choice of using methods that are extremely reminiscent of app state / viewport. To me when I started converting some of my stuff to controls, my first impression was “It’s the same as app state, but for spatials.” but was rewarded with a behavior that is different.



In the end I guess we’ll have to agree to disagree.

1 Like