A more convenient way to get a Spatial's control list

General Discussion
1

Hi all,
Could a more convenient way to get the list of controls be added to the Spatial class?

For example:

public class Spatial ... {

        protected SafeArrayList<Control> controls = new SafeArrayList<>(Control.class);

        // Something like this...
        public SafeArrayList<Control> getControls() {
            return controls;
        }
        
        // ...or this...
        public Control[] getControls() {
            return controls.getArray();
        }
}

Or is there a specific reason why now you can only iterate the controls list with an indexed for?

for (int i = 0; i < spatial.getNumControls(); i++) {
        Control control = spatial.getControl(i);
        // do something...
}

WDYT?

2

Handing out lists is always a little bit dodgy because it allows other classes to add or remove from them without control of the owning class.

A unmodifiable view might help with that a bit (although it leads to a type signature that lies and fails at run time which I dont love)

3

I should point out that it is already possible to access the localOverrides and worldOverrides (which are SafeArrayLists like the controls) via their respective getter methods.

public SafeArrayList<MatParamOverride> getLocalMatParamOverrides() {

public SafeArrayList<MatParamOverride> getWorldMatParamOverrides() {
4

That’s because there is no maintenance to be done.

Adding/removing controls cannot be done through a returned list. Absolutely not. So it’s safer not to allow it.

Also, it almost never comes up in normal use.

3 Likes
5

Thanks for your answers guys.

I don’t quite understand what you mean. If, for example, you access the localOverrides list via the getLocalMatParamOverrides method and add elements without using the addMatParamOverride method (which contains the call to the setMatParamOverrideRefresh method) you already risk messing up the code. Same thing for the lightList (getLocalLightList; addLight internally calls setLightListRefresh).

Spatial sp = ...;
MatParamOverride mpo = ...;
sp.getLocalMatParamOverrides().add(mpo); 
// Currently you can do this, but it is a bad practice 
// and can cause unexpected behavior. 

// Perhaps the methods getLocalMatParamOverrides() and getLocalLightList(), 
// were added in the past accepting the risk of forbidden changes, 
// to avoid creating an immutable copy of the list in an internal method called often.

I know the basic rules of programming, too, but the current code situation is already ambiguous and does not comply with them. Obviously, since the purpose is not to add/remove elements from the returned list, but to quickly access list elements (perhaps via foreach and concatenated functions (streams, filters, etc…)) to create editors, I think a good compromise might be to add a method that returns an unmodifiable version of the control list, taking a cue from the FilterPostProcessor.getFilterList method. If you think it would be useful I will send a PR, otherwise no problem.

class Spatial ... {
    /**
     * returns an unmodifiable version of the control list.
     * @return the controls list
     */
    public List<Control> getControlList(){
        return Collections.unmodifiableList(controls);
    }
}
6

CONTROLS cannot operate properly without having the proper life cycle. It’s less about the LIST. You can add as many material parameter overrides manually to the list that you want as long as something eventually sets the modified bit. This is not at all the same as controls.

Sure. For that 0.00000001% of use-cases that would appropriate use it, it’s probably fine. JME generally tries to avoid creating garbage unnecessarily, though. But given almost no one should actually use this method, maybe it’s ok.

The only reservations I have is users not realizing there is already a better way to do what they want for 99.9999999% of cases. And the standard excuse that “every line of code adds to the maintenance burden”.

These are also the reasons JME usually doesn’t add “nice for editor tools” style additions gumming up the API. I’m making a big assumption assuming that’s why you want it in this case.