Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InterfaceGl: named params + callbacks #419

Merged
merged 23 commits into from
Apr 26, 2014
Merged

InterfaceGl: named params + callbacks #419

merged 23 commits into from
Apr 26, 2014

Conversation

richardeakin
Copy link
Collaborator

This supersedes #387. @Numeric thanks for the initiative continued work on this.

This adds some missing functionality to params::InterfaceGl, as well as missing doxygen docs. List of source changes:

Options can now be added using the named parameter idiom:

mParams->addParam( "Cube Size", &mObjSize ).min( 0.1f ).max( 20.5f ).keyIncr( "z" ).keyDecr( "Z" ).precision( 2 ).step( 0.02f );

You can add an update callback as an option that is fired after a target is updated by ATB:

mParams->addParam( "some value", &mSomeValue ).updateFn( [this] { console() << "new value: " << mSomeValue << std::endl; } );

You can also add a setter + getter, which does not require a target to bind to:

std::function<void( Vec3f )> setter = std::bind( &TweakBarApp::setLightDirection, this, std::placeholders::_1 );
std::function<Vec3f ()> getter      = std::bind( &TweakBarApp::getLightDirection, this );
mParams->addParam( "Light Direction", setter, getter );

Andrew, note that this is different from what you reviewed the other day; while you could also manage this with the other addParam function (and passing in null for the target pointer), that way is a bit confusing interface wise. This method is clearer IMO: you don't need a target, you need two std::function's instead.

I've added all possible types that ATB supports, there were a couple left out before like uint8_t, uint16_t, etc.

The old addParam() methods that take an optionsStr are still in there, though marked deprecated.

Sample has been updated, a few more usage examples were added for the callback stuff.

@num3ric
Copy link
Contributor

num3ric commented Apr 20, 2014

This method is clearer IMO: you don't need a target, you need two std::function's instead.

I agree, the whole point of having a setter/getter is to avoid exposing a variable (target).

The only problem (which my solution also suffered from and had been solved by your previous templated Param approach) is that you can't pass the std::bind setter/getter functions directly without typecasting them. Similarly, you couldn't write auto setter = std::bind(...

I'd move forward with this regardless. It's very much needed!

@richardeakin
Copy link
Collaborator Author

You can simplify it a bit, although you need to specify the type in the method template since it can't be inferred:

auto setter = std::bind( &TweakBarApp::setLightDirection, this, std::placeholders::_1 );
auto getter     = std::bind( &TweakBarApp::getLightDirection, this );
mParams->addParam<Vec3f>( "Light Direction", setter, getter );

You actually had to do this with my previous approach as well, which looked like:

mParams->addParam<Vec3f>( "Light Direction" ).accessors( setter, getter );

Here, the target param defaulted to nullptr, which also doesn't allow the type to be inferred by the compiler. But there are two weaknesses:

a) the auto-completion makes the user think there is only one required parameter, and
b) if the .accessors() part is left off, nothing happens at all and it isn't obvious why not.

So yea for the case of setter + getter, I think this is fitting. However, in the case of an update callback, I prefer the way it feels with it chained.

@num3ric
Copy link
Contributor

num3ric commented Apr 21, 2014

I was referring to your first test with Param< T > but I forgot you could specify the type yourself, ding, ding! Nice.

richardeakin added a commit that referenced this pull request Apr 26, 2014
InterfaceGl: named params + callbacks
@richardeakin richardeakin merged commit 0e3411d into dev Apr 26, 2014
@richardeakin richardeakin deleted the params_named branch May 12, 2014 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants