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

Add more intuitive geom::Combine() syntax #700

Open
sansumbrella opened this issue Feb 9, 2015 · 4 comments
Open

Add more intuitive geom::Combine() syntax #700

sansumbrella opened this issue Feb 9, 2015 · 4 comments
Labels

Comments

@sansumbrella
Copy link
Contributor

The upstream + source behavior of geom::Combine() is confusing.

Consider adding a geom::Combine() method that is more intuitive:

auto cube = geom::Cube();
auto sphere = geom::Sphere();
// Proposed
auto together = geom::Combine(&cube, &sphere);
// Current (I think)
auto together = cube >> geom::Combine(&sphere);

It would be extra cool if it was written with variadic parameters, so you could combine as many things as you like.

Alternatively, consider renaming geom::Combine() to geom::Append() to more clearly describe what it is doing: adding a piece of geometry at the end of the stream (right?).

@sansumbrella
Copy link
Contributor Author

I will try putting together a PR for this, but I'm not yet familiar with the inner workings of the geom pipeline.

EDIT: looks like what I'm thinking of should be a Source, not a Modifier. Starting down the road of an Aggregate Source.

@andrewfb
Copy link
Collaborator

andrewfb commented Feb 9, 2015

I'm open to this but maybe it's worth considering a name change rather than adding additional code. It seems like a new Source type is not adding any functionality we don't already have, in the sense that the example you cite above (proposed and current) are indeed equivalent. Part of the advantage of the current design is that the Combine()d geometry is secondary. This is important when deciding what to do when one of the sources lacks attributes the other does not, and is part of why I went with this design.

@sansumbrella
Copy link
Contributor Author

That makes sense.

How do you feel about changing to geom::Append() then?

@heisters
Copy link
Contributor

Apologies if this is not related, but I would love to see something that would allow using geom::Combine() with a source argument by value, and return a new source. I've been trying to turn this:

    auto b1 = geom::Cube() >> geom::Rotate( quat( randVec3f() * (float)M_PI * 2.f ) ) >> geom::Scale( randVec3f() );
    auto b2 = geom::Cube() >> geom::Rotate( quat( randVec3f() * (float)M_PI * 2.f ) ) >> geom::Scale( randVec3f() );
    auto b3 = geom::Cube() >> geom::Rotate( quat( randVec3f() * (float)M_PI * 2.f ) ) >> geom::Scale( randVec3f() );
    auto b4 = geom::Cube() >> geom::Rotate( quat( randVec3f() * (float)M_PI * 2.f ) ) >> geom::Scale( randVec3f() );
    auto b5 = geom::Cube() >> geom::Rotate( quat( randVec3f() * (float)M_PI * 2.f ) ) >> geom::Scale( randVec3f() );
    auto b6 = geom::Cube() >> geom::Rotate( quat( randVec3f() * (float)M_PI * 2.f ) ) >> geom::Scale( randVec3f() );
    auto b7 = geom::Cube() >> geom::Rotate( quat( randVec3f() * (float)M_PI * 2.f ) ) >> geom::Scale( randVec3f() );

    gl::VboMesh::create( b1 >> geom::Combine( &b2 ) >> geom::Combine( &b3 ) >> geom::Combine( &b4 ) >> geom::Combine( &b5 ) >> geom::Combine( &b6 ) >> geom::Combine( &b7 ) );

into something like this (which doesn't compile):

    auto boxy = geom::Cube();
    for ( size_t i = 0; i < 10; ++i ) {
        auto b = geom::Cube() >> geom::Rotate( quat( randVec3f() * (float)M_PI * 2.f ) ) >> geom::Scale( randVec3f() );
        boxy = b >> geom::Combine( &boxy );
    }
    gl::VboMesh::create( boxy );

I've tried a bunch of other possibilities, but they all fail at compile time or runtime. The latter, ostensibly because the argument to geom::Combine() is out of scope when the target requests the geometry.

@paulhoux paulhoux added the math label Nov 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants