openframeworks / openFrameworks

openFrameworks is a community-developed cross platform toolkit for creative coding in C++.
http://openframeworks.cc
Other
9.98k stars 2.55k forks source link

feature : recode ofNode to use parameters #1426

Open elliotwoods opened 12 years ago

elliotwoods commented 12 years ago

Hey all!

I need a way of having the translate, rotate and scale of an ofNode (essentially all the acknowledged parameters) act as parameters.

My suggestion before was to make parameters able to use external values (e.g. you can make your own ofParameter-type class that doesn't internally store an ofVec3f, but instead gets and sets the position of an ofNode. with that class being able to sit within the inheritance tree of ofParameter, and therefore be tied to a slider, etc). This suggestion didn't get positive response as it complicates ofParameter

The next suggestion is to recode ofNode to internally use 3 parameters:

ofParameter<ofVec3f> translation;
ofParameter<ofQuaternion> rotation;
ofParameter<ofVec3f> scale;

you could then bind external sliders to these parameters we keep the existing API the biggest difference would be that you couldn't apply a projective transform to an ofNode (if you applied a matrix, we'd perform some decomposition to make it back into a Tra,Rot,Sca

the alternative so far has been to cache values somewhere, and have an update loop that checks for changes inside the ofNode i'd like to avoid this as it's very messy (e.g. who does this update, what is the epsilon value, etc)

this isn't an easy ask, but the type of control that it unlocks is really powerful so your consideration is appreciated

ofTheo commented 12 years ago

hmmmmmmmmmmm. :)

my general feeling is not to make ofNode dependent on ofParam. ofNode is such a general base type that making it use another newly introduced data type, kind of doesn't feel right.

maybe @arturoc could share his thoughts?

In theory having ofParam able to work with external data could be worth looking at as it would then allow for a lot of flexibility.

another thing could be a a

ofBaseHasParams::getParams();

so:

ofNode::getParams();

returns a param list which is constructed on the fly with the params having internal references to the ofVec and ofQuaternion data? this could work really well!

obviousjim commented 12 years ago

Is there a way to do ofParameter<ofVec3f&> pTranslation, etc, so that the parameter is referencing the same field as ofVec3f translation. That way you could modify both?

On Fri, Jul 27, 2012 at 5:43 AM, Elliot Woods < reply@reply.github.com

wrote:

Hey all!

I need a way of having the translate, rotate and scale of an ofNode (essentially all the acknowledged parameters) act as parameters.

My suggestion before was to make parameters able to use external values (e.g. you can make your own ofParameter-type class that doesn't internally store an ofVec3f, but instead gets and sets the position of an ofNode. with that class being able to sit within the inheritance tree of ofParameter, and therefore be tied to a slider, etc). This suggestion didn't get positive response as it complicates ofParameter

The next suggestion is to recode ofNode to internally use 3 parameters:

ofParameter<ofVec3f> translation;
ofParameter<ofQuaternion> rotation;
ofParameter<ofVec3f> scale;

you could then bind external sliders to these parameters we keep the existing API the biggest difference would be that you couldn't apply a projective transform to an ofNode (if you applied a matrix, we'd perform some decomposition to make it back into a Tra,Rot,Sca

the alternative so far has been to cache values somewhere, and have an update loop that checks for changes inside the ofNode i'd like to avoid this as it's very messy

i need something which isn't an easy ask, so your consideration is appreciated


Reply to this email directly or view it on GitHub: https://github.com/openframeworks/openFrameworks/issues/1426

arturoc commented 12 years ago

The problem with parameters is that they are slower than simply accessing the variable cause they are triggering events and doing some checks so i won't use them by default for something like ofNode which should be as fast as possible

The 2 solutions could be callbacks or having parameters that reference an external variable

El 27/07/12 02:11, James George escribió:

Is there a way to do ofParameter<ofVec3f&> pTranslation, etc, so that the parameter is referencing the same field as ofVec3f translation. That way you could modify both?

On Fri, Jul 27, 2012 at 5:43 AM, Elliot Woods < reply@reply.github.com

wrote:

Hey all!

I need a way of having the translate, rotate and scale of an ofNode (essentially all the acknowledged parameters) act as parameters.

My suggestion before was to make parameters able to use external values (e.g. you can make your own ofParameter-type class that doesn't internally store an ofVec3f, but instead gets and sets the position of an ofNode. with that class being able to sit within the inheritance tree of ofParameter, and therefore be tied to a slider, etc). This suggestion didn't get positive response as it complicates ofParameter

The next suggestion is to recode ofNode to internally use 3 parameters:

ofParameter<ofVec3f> translation;
ofParameter<ofQuaternion> rotation;
ofParameter<ofVec3f> scale;

you could then bind external sliders to these parameters we keep the existing API the biggest difference would be that you couldn't apply a projective transform to an ofNode (if you applied a matrix, we'd perform some decomposition to make it back into a Tra,Rot,Sca

the alternative so far has been to cache values somewhere, and have an update loop that checks for changes inside the ofNode i'd like to avoid this as it's very messy

i need something which isn't an easy ask, so your consideration is appreciated


Reply to this email directly or view it on GitHub: https://github.com/openframeworks/openFrameworks/issues/1426

elliotwoods commented 12 years ago

Great I would prefer external parameters Also event callbacks on ofNode (as mentioned above) would be great I was imaging something like

class ofNode {
....
    ofEvent<ofVec3f> onTranslate;
    ofEvent<ofQuaternion> onRotate;
    ofEvent<ofVec3f> onScale;
...
};

Note the 'on' would be a new naming schema, to be replaced with an agreed standard of course.

elliotwoods commented 12 years ago

bump!! so which is it to be external variables or events inside ofNode?

dropping ofParameters into ofNode itself is the quickest to develop, but notably somewhat slows down nodes (although personally i think a scene probably shouldn't need more than 1000 nodes anyway, after which the usage case has significantly changed from what I've always believed ofNode is designed for. Given <1000 nodes per frame, we wouldn't experience noticeable slow-down)

ofTheo commented 12 years ago

@elliotwoods

What about the following ( warning psudo code ):

class ofNode : public ofBaseHasParams{

    public:

        ofParamList getParams(){
            ofParamList params;

            params.setGroupName("ofNode"); 

            //ofParameter::makeParam returns a constructed ofParameter with a reference to an external variable / type/ 
            params.push_back( ofParameter::makeParam( pos, "position" ) ); 
            params.push_back( ofParameter::makeParam( scale, "scale" ) ); 
            params.push_back( ofParameter::makeParam( rotation, "rotation" ) ); 

            return params;
        }
        ......

        ofVec3f pos;
        ofVec3f scale;
        ofQuaternion rotation;

        etc....
};

The idea is we don't have any ofParameters in the ofNode class but we can turn the ofNode data into a parameter list as needed on the fly. This would be a compromise of sorts as you wouldn't have the performance issues with all the events firing, but you could still have a little more control over how ofNode is defined in terms of parameters.

Another option could be your original suggestion:

    ofParameter<ofVec3f> translation;
    ofParameter<ofQuaternion> rotation;
    ofParameter<ofVec3f> scale;

But with events disabled by default. Events could be activated by the gui system when the node is first added. Or manually turned on / off.

elliotwoods commented 12 years ago

if we have ofVec3f pos and a parameter, then when we translate, how is the parameter notified? or does ofNode then use the parameter instead of the local variable? (i.e. no point in having both)

i.e. externally allocated parameters allow us to tie a parameter to an external value, but if we change the external variable directly the parameter is not notified. This is acceptable in some situations, but probably would be a bad move in the core as it could be confusing. (That said, I'd be happier if such a mechanism existed for users to access, but it's a low priority)

So Theo's suggestion above would work for me, but not all use cases.

+1 to reconsider

    ofParameter<ofVec3f> translation;
    ofParameter<ofQuaternion> rotation;
    ofParameter<ofVec3f> scale;

We need to determine

if (the performance gain is negligible && the capability gain is important){
    replaceVarsWithParams(); // original suggestion
} else if(we accept not needing to trigger event when function is called) {
    useExternalParams(); // like theo's suggestion
} else {
   addCallbackEvents(); // e.g. onTranslate, onRotate, onScale or just onChange
   //callbacks could perhaps be toggled with a bool
}

summon: @memo

arturoc commented 12 years ago

the main problem i see with this is why do it in ofNode? i mean it seems like a very random class to have parameters, why not ofVideoPlayer or Grabber..., perhaps we should talk about if we want something like this in the core and in which kind of classes.

El 30/07/12 17:56, Theodore Watson escribió:

@elliotwoods

What about the following ( warning psudo code ):

class ofNode : public ofBaseHasParams{

  public:

      ofParamList getParams(){
          ofParamList params;

          params.setGroupName("ofNode");

          //ofParameter::makeParam returns a constructed ofParameter with a reference to an external variable / type/
          params.push_back( ofParameter::makeParam( pos, "position" ) );
          params.push_back( ofParameter::makeParam( scale, "scale" ) );
          params.push_back( ofParameter::makeParam( rotation, "rotation" ) );

          return params;
      }
      ......

      ofVec3f pos;
      ofVec3f scale;
      ofQuaternion rotation;

      etc....

};

The idea is we don't have any ofParameters in the ofNode class but we can turn the ofNode data into a parameter list as needed on the fly. This would be a compromise of sorts as you wouldn't have the performance issues with all the events firing, but you could still have a little more control over how ofNode is defined in terms of parameters.

Another option could be your original suggestion:

  ofParameter<ofVec3f> translation;
  ofParameter<ofQuaternion> rotation;
  ofParameter<ofVec3f> scale;

But with events disabled by default. Events could be activated by the gui system when the node is first added. Or manually turned on / off.


Reply to this email directly or view it on GitHub: https://github.com/openframeworks/openFrameworks/issues/1426#issuecomment-7372480

elliotwoods commented 12 years ago

evidently a lot of ofClasses could see benefit from using parameters we can either:

  1. Use parameters where suitable and we can agree on performance not being an issue
  2. Not use parameters in the core

Perhaps we're not really ready for 1 yet as ofParameter hasn't had enough field testing / nailing down But I'd love it if we went in that direction over the coming releases.

ofTheo commented 12 years ago

Perhaps we're not really ready for 1 yet as ofParameter hasn't had enough field testing / nailing down

But I'd love it if we went in that direction over the coming releases.

good point! but I agree it really becomes powerful when things that will be often hooked up to guis / settings are assigned as ofParameters or easily made into them.

elliotwoods commented 12 years ago

Speed tests

Overview

Test speed of ofNode when internal variables are replaced with ofParameter. We use the default ofNode::draw() which is a simple box and 3 lines.

Note, ofNode::orientation isn't parameterised in this test as ofQuaternion doesn't support stream operations (and therefore can't be a parameter).

Code

#include "testApp.h"

void stepTimer(float & running, const clock_t & now) {
    float nowSeconds = float(now) / (float)(CLOCKS_PER_SEC * COUNT);
    if (running == 0.0f)
        running = nowSeconds;
    else
        running = running * 0.9f + nowSeconds * 0.1f;
}

//--------------------------------------------------------------
testApp::testApp(){
    this->setupTime = 0.0f;
    this->updateTime = 0.0f;
    this->drawTime = 0.0f;
}

//--------------------------------------------------------------
void testApp::setup(){
    glEnable(GL_DEPTH_TEST);
    ofBackground(20);

    nodes.clear();

    clock_t start = clock();

    for (int i=0; i<COUNT; i++){
        nodes.push_back(ofNode());
        nodes.back().setPosition(ofRandom(-1000.0f, 1000.0f), ofRandom(-1000.0f, 1000.0f), ofRandom(-1000.0f, 1000.0f));
    }

    stepTimer(this->setupTime, clock() - start);
}

//--------------------------------------------------------------
void testApp::update(){
    clock_t start = clock();

    vector<ofNode>::iterator it;
    for (it = nodes.begin(); it!= nodes.end(); it++) {
        it->rotate(10, it->getPosition().normalized().getPerpendicular(ofVec3f(1,0,0)));
        it->move(-it->getPosition() * 0.001f);
    }

    stepTimer(this->updateTime, clock() - start);
}

//--------------------------------------------------------------
void testApp::draw(){
    clock_t start = clock();

    camera.begin();
    vector<ofNode>::iterator it;
    for (it = nodes.begin(); it!= nodes.end(); it++)
        it->draw();
    camera.end();

    stepTimer(this->drawTime, clock() - start);

    cout << "Setup: " << this->setupTime << "\tUpdate: " << this->updateTime << "\tDraw: " << this->drawTime << "\t(s)" << endl;
}

//--------------------------------------------------------------
void testApp::keyPressed(int key){
    this->setup();
}

Results

Times are in seconds and averaged per node over many function calls.

Method Time per node (s) Time per parameterised node (s)
Setup 3.99e-7 2.14e-6
Update 3.03e-7 6.53e-7
Draw 8.66e-6 8.76e-6

 Analysis

Method Increase in compute time Times can be performed per frame at 60fps
Instantiation 10x 7,000
Move 2x 25,000
Draw 1% 2,000
ofTheo commented 12 years ago

thanks for doing the tests elliot!

wouldn't my suggestion solve the slowdown issue? ie: keep everything as it is, but have getParams as a self contained method that constructs a temporary list and returns it when its needed. the params in the list would have pointers to the external data ( ofVec3fs in ofNode ), so you still get quick param access and no slowdown from accessing the data.

arturoc commented 12 years ago

perhaps disabling the events somehow as theo suggest makes that times slightly better. apart from the event there's an if and a boolean flag to avoid feedback loops that can be time consuming too.

another option could be having an ofParameter and some special gui element for that.

for other classes or parameters where performance is not so critical i agree that it will be super useful to have this, but it seems kind of a very particular pattern, i haven't seen anything like this anywhere, perhaps we could have something that allows to assign a parameter to a class by passing the set/get functions of the particular parameter we want to monitorize through an ofParameter so the implementation is not so invasive

El 30/07/12 19:45, Elliot Woods escribió:

Also to note (not taking into account above), is that if we have an ofParameterGroup inside ofNode (i.e. ofParameterGroup ofNode::parameters) then that would also increase instantiation time.


Reply to this email directly or view it on GitHub: https://github.com/openframeworks/openFrameworks/issues/1426#issuecomment-7375811

elliotwoods commented 12 years ago

EDIT: this post is in response to theo's

If we created parameters like that, that's an implementation of the 'external parameters' idea (i.e. parameters can reference external variables, or even have arbitrary get/set functions)

The issue with referencing external vars, is that if I call ofNode::move(...), then it will change ofVec3f position but not trigger an update callback on the parameter. This is fine for gui's (they generally read the live value), but doesn't work with anything that needs the callback.

As can be seen above, access isn't really an issue (setting the value takes 3e-7 longer, whilst getting only takes 1e-7 longer).

elliotwoods commented 12 years ago

Before going any further, can we examine the results more. Yes there is an increase in compute time (there had to be) But does it actually rule out the initial suggestion?

I'd personally say that

  1. ofNode doesn't need to be 'quick', because it isn't designed to deal with 1000's of nodes per scene
  2. ofNode is still quick with the ofParameter changes
elliotwoods commented 12 years ago

ok, so in response to the above should we move the discussion over to something like ofVideoPlayer where the conept of 'the speed of isPlaying variable' is too ridiculous to detract from the rest of the conversation about ofParameter implementation (although I will come back to ofNode eventually!)

arturoc commented 12 years ago

i've commited a change that allows to disable the events for a parameter, with that the performance should be the same as without parameter, except for instantiation (i've changed that too, now it should be slightly faster). if you can run the tests again just out of curiosity

just call parameter.disableEvents()

and yes, i think it makes sense to discuss if we want to use this pattern in the core at all then check in which cases it makes sense.

El 30/07/12 20:14, Elliot Woods escribió:

ok, so in response to the above should we move the discussion over to something like ofVideoPlayer where the conept of 'the speed of isPlaying variable' is too ridiculous to detract from the rest of the conversation about ofParameter implementation (although I will come back to ofNode eventually!)


Reply to this email directly or view it on GitHub: https://github.com/openframeworks/openFrameworks/issues/1426#issuecomment-7376701

elliotwoods commented 12 years ago

woops. accidentally deleted the note above about not using the group (one of mine) when i was taking computer out of standby. will check now with your latest

elliotwoods commented 12 years ago

I've reset my computer between doing these tests, but the new results (with events disabled on scale and position): Setup: 1.1307e-06 Update: 3.60089e-07 Draw: 8.95063e-06 (s)

The update time decreases to nominally the same as before (only a 20% increase) The draw time increases (although this could be other effects) Setup time decreases (this is because I'm doing some setting of positon in setup, so this is due to a decrease in set time)

elliotwoods commented 12 years ago

so after that change, instantiation is still a little slower (there's more stuff to create, and params to disable, and eventually a group to create). but get and set times have no notable difference

we have to be careful that these times could increase though as ofNode becomes more mature

arturoc commented 12 years ago

great, i've also inlined the setters and getters for ofParameter in a later commit which should decrease even more the times but that results seem enough, the discussion i guess is if we really want this kind of pattern in the core or what kind of problems can it have.

the main problem i see is that having a public parameter is almost like having a public variable which is not a very good practice, since parameters have events they can actually be treated as a setter but the code can be messy. also we could add a getter event so when you try to read the value it actually calls a function to return a calculated value, which should solve the problem of having it as a public member completely

bilderbuchi commented 12 years ago

just btw, doesn't inlining happen automatically anyway on modern compilers? At least that's what I've read quite a couple of times already...

ofTheo commented 12 years ago

or ofParams stay protected and getParams() returns the param list?


yeah agreed @arturo. this would be another argument for ofParmas only being pointers to data. rather than data itself. ofBaseHasParams ( which ofNode would inherit ) would be a friend to ofParameter class so it could access the protected variables ( ofVec3fs etc ).

Then getParams() would get the param list for the gui.

Still doesn't help with the callback issue - though not sure if we can have all these things at the same time :)

elliotwoods commented 12 years ago

So to summarise we've got 2 scenarios:

1. ofClasses which can be entirely driven internally by parameters

Examples

Situation

We can drop-in-replace existing variables with parameters, and give the user access to these parameters with ofParameterGroup ofClass::getParameters(), which also automatically enables the events on the internal parameters.

2. ofClasses which require callbacks on parameter changes

 Examples

 Situation

We need to perform an action whenever a parameter is changed. This means that if the parameters are public in any way (i.e. this is the case if we want to use parameters here), then we need to call the correct line of functions.

A simple example of this would be:

  1. Existing get/set interface with the parameter and do not perform any functionality (e.g. actually stopping a movie from playing back)
  2. When the parameter is changed, callbacks perform necessary functions (e.g. stopping movie playback).

This is a little slower per call, but again, these aren't entirely time-critical type actions (e.g. delaying the beginning of a sound by 1e-6 seconds isn't going to upset anyone).

elliotwoods commented 12 years ago

can somebody tell me why my syntax isn't working above i'm getting this a lot recently (github syntax failing)

arturoc commented 12 years ago

i've done some changes in the demo PR related to this will put comments there

kylemcdonald commented 9 years ago

this is very old and i don't see a corresponding PR from @arturoc did this get resolved, or no?

elliotwoods commented 9 years ago

hey! wow just found this searching to see if we'd had this discussion before :)

is there any interest in this after it's settled a bit?

to summarise the above:

My instinct is that ofNode and ofCamera are the best examples where to start with this. Something like ofSoundPlayer (essentially something which manages a 'device' seems best to avoid parameters).

You could override ofNode and add your own parameters.

Adapting theo's suggestion, the user could do:

auto & parameters = this->camera.getParameters();
this->camera.enableParameterCallbacks();
this->gui.add(parameters);

or define the function :

virtual ofParameterGroup & ofNode::getParameters(bool enableParameterCallbacks = true);
chuckleplant commented 8 years ago

Hi all, I've been tracking ofParamter usage and it brought me here. I think this is now closed by this commit. So I'm guessing this discussion finished somewhere else.

To add my 2c @arturoc 's initial point that ofParameter might not belong inside ofNode seems to have lost its weight (should it?). I don't know what's the overall cost of the ofParameter events, even when disabled. But it might be worth (imho) considering the ofBaseHasParams, which could force any class that implements it to return an ofParameterGroup attached to the class's listeners. This could decouple ofParameter from the model class a bit more.

In any case, now that we have the onParentPositionChanged this could help tackle the matrix caching proposal from some time back.

To finish with, it's worth considering how serialization plays with the mix of placing ofParameter inside core oF classes. Seems like ofParameter will eventually eat up all of the core's attributes that could end up in the GUI.

bakercp commented 7 years ago

@elliotwoods Can you make the call on whether this issue should be closed?