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

Implement btIDebugDraw for Magnum #21

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
3 participants
@Squareys
Contributor

Squareys commented Oct 2, 2016

Hi @mosra !

As promised via gitter, here is the PR for the bullet physics visualization ( #11 ).

There is one // TODO in draw3dText(...), you might want to look at how to specify/format it instead. I could also adapt the bullet example and whilst I'm at it remove the Shapes dependecy from it (since it will be deprecated) once you merged this.

Regards, Squareys.

@coveralls

This comment has been minimized.

coveralls commented Oct 2, 2016

Coverage Status

Coverage remained the same at 0.0% when pulling be5509b on Squareys:physics-debug-draw into cdace8d on mosra:master.

@Squareys

This comment has been minimized.

Contributor

Squareys commented Oct 2, 2016

Coverage remained the same at 0.0%

Uh... I guess it's not set up for magnum-integration yet?

@mosra

This comment has been minimized.

Owner

mosra commented Oct 3, 2016

No. If you remember, that's STILL because of this: travis-ci/apt-package-safelist#1190

I'm getting angry just seeing that issue.

The Bullet integration lib is built for OSX, but there I don't have the code coverage enabled. Maybe I should give up on waiting and build Bullet from source in this case.. :/

@Squareys

This comment has been minimized.

Contributor

Squareys commented Oct 3, 2016

No. If you remember, that's STILL

Right, forgot about it, it's been a year since.

Any other comments on the PR? For once it is not WIP, was simple enough ;) (Not pushing, would be great to have this upstream, though.)

#undef _c
}
return debug << "BulletIntegration::DebugDraw::Mode::(invalid)";

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor

Alright, will do 👍

  • done
*/
/** @file
* @brief Implementation of @ref btIDebugDraw for physics visualization in bullet

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

The @ref works only when referencing things inside the project, not for types from outside.

Also, Bullet, not bullet, please ;)

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor

Ok, hence: Remove the @ref and have "Bullet" with capital B.

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor
  • done
#include <Magnum/Mesh.h>
#include <Magnum/Shaders/VertexColor.h>
#include <bullet/LinearMath/btIDebugDraw.h>

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

The bullet/ prefix won't work everywhere I fear. Use just <LinearMath/btIDebugDraw.h>.

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor

Ok, did not know that, will do.

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor
  • done

Was not aware of that 👍

DrawFrames = DBG_DrawFrames, /**< Draw frames */
};
typedef Containers::EnumSet<Mode> Modes;

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

Please include a Doxygen block for this type as well.

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor

/** @brief Debug draw mode flags */ ? :)

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

Yes, and "flag" instead of "enum" for the Mode enum.

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor
  • done
{
_mesh.addVertexBuffer(_buffer, 0, Shaders::VertexColor3D::Position{}, Shaders::VertexColor3D::Color{});
_bufferData.reserve(initialBufferCapacity*4);
}

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

I would move the constructor to the source file, as it is quite heavy (std::vector, vtables...) and makes the compiler generate the code inline every time. I would also add an explicitly defaulted destructor to the source file for similar reasons.

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor

So... instead of providing a default value for initialCapacity parameter, right?

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor
  • done
}
/** @overload */
virtual void setDebugMode(Modes debugMode) {

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

This is not overriding anything and nobody will ever override it so it doesn't have to be virtual. Correct me if I'm wrong :)

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor

Nope, totally right, found that just now myself also 👍

  • done
}
/** @brief Draw text */
virtual void draw3dText(const btVector3& CORRADE_UNUSED location, const char* CORRADE_UNUSED textString) override {

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

Just omit the argument name completely in case of unused parameters:

void draw3dText(const btVector3&, const char*) override {

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor

But some day you may want to implement it and then you would need to look up what the paramters mean? :)

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

Good point :)

Assuming the declaration is in header and implementation in the source file, the header can retain the parameter names and the implementation can have them omitted. That way you can always look back to the header for the names.

}
/** @brief Flush lines to be drawn on screen */
virtual void flushLines() override;

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

I assume that all overriden functions are called only from Bullet and not from user code. Could you make them all private to hide them from documentation?

The only public interface would then be setDebugMode(Mode) and setViewProjectionMatrix().

Or is there a valid use case for calling them directly?

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

Also, similarly to the constructor and destructor, I would move all the virtual function overrides into the *.cpp file. The compiler has to de-inline them anyway and by doing that explicitly you save some more compilation time when just including the header.

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor
  1. done
  2. done
virtual void flushLines() override;
/** @brief Set view projection matrix used for rendering */
DebugDraw& setViewProjectionMatrix(const Matrix4& viewProj) {

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

According to naming scheme everywhere else I would name this function (and related variables/docs) setTransformationProjectionMatrix().

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor
  • done
_bufferData.push_back(Vector3(from));
_bufferData.push_back(Color3::from(fromColor));
_bufferData.push_back(Vector3(to));
_bufferData.push_back(Color3::from(toColor));

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

First: I realized that there is no way to create Color from an external representation. That I have to fix. Sorry.

Second: the only ::from() I found is one that takes T*. And now I'm wondering, how the hell does this even compile? :D MSVC?!

This comment has been minimized.

@Squareys

Squareys Oct 3, 2016

Contributor

Yes, MSVC. I wondered, too. I will use the underlying float array, which is public.

This comment has been minimized.

@mosra

mosra Oct 3, 2016

Owner

Commit mosra/magnum@b033af3 fixes the Color conversion so you can now make it the same way as with Vector3.

@mosra

This comment has been minimized.

Owner

mosra commented Oct 3, 2016

Review done :)

The OSX build fails because of missing Shaders dependency. Can you update the CI files accordingly?

Regarding the text rendering -- yes, as you suggest, i think the better option long-term would be to use the "debug text" functionality once it's in. The TODO is fine like this. Or, maybe, put it in a Doxygen @todo block.

Until then, if you feel like doing it (just a suggestion!), even just copypasting the guts of the Text example and having the constructor take an optional Text::AbstractFont* font pointer with an opened font -- i.e. assert on it like this:

CORRADE_INTERNAL_ASSERT(!font || font->isOpened());

If the user won't supply the parameter (the default), it will not render any text. If the user provides the text, it will render it.

Eh, scratch that, now realized that this will be kinda hard to do with this "immediate" approach of the interface, as there is no flushText() or anything similar. Would need to think about it a bit more.

@coveralls

This comment has been minimized.

coveralls commented Oct 3, 2016

Coverage Status

Coverage remained the same at 0.0% when pulling 908642c on Squareys:physics-debug-draw into cdace8d on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Oct 3, 2016

Coverage Status

Coverage remained the same at 0.0% when pulling 404c308 on Squareys:physics-debug-draw into cdace8d on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Oct 3, 2016

Coverage Status

Coverage remained the same at 0.0% when pulling 404c308 on Squareys:physics-debug-draw into cdace8d on mosra:master.

Squareys added some commits Oct 2, 2016

BulletIntegration: Implement btIDebugDraw as DebugDraw
Signed-off-by: Squareys <Squareys@googlemail.com>
BulletIntegration: Add test for DebugDraw::Mode enum
Signed-off-by: Squareys <Squareys@googlemail.com>
BulletIntegration: Cleanup code
 - Enhance DebugDraw::Mode debug operator
 - Use `` instead of refs for Bullet types in doc
 - Remove bullet/ prefix for Bullet headers
 - Also document Modes EnumSet
 - Move method, constructor and destructor definitions to source
 - Remove virtual keyword
 - Have all overrides private, since only called by Bullet
 - Rename viewProjection to transformationProjection
 - Use conversion from external representation for Color
 - Remove CORRADE_UNUSED in source files.

Signed-off-by: Squareys <Squareys@googlemail.com>
ci: Enable building WITH_SHADERS when building BulletIntegration
Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:physics-debug-draw branch from 404c308 to f9d1716 Oct 3, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 3, 2016

Coverage Status

Coverage remained the same at 0.0% when pulling f9d1716 on Squareys:physics-debug-draw into b59c764 on mosra:master.

@mosra

This comment has been minimized.

Owner

mosra commented Oct 4, 2016

Cherry-picked as f8eb01d, 28eb3c7 and de3b54e. On Linux the test failed the same way as on OSX, so I added an explicit cast to UnsignedInt before casting to void* to avoid sign extension.

@Squareys

This comment has been minimized.

Contributor

Squareys commented Oct 4, 2016

What's with a08be62? (Ah, I see that was also squashed into one of the other commits)

@mosra

This comment has been minimized.

Owner

mosra commented Oct 4, 2016

Into both, actually :)

I'm leaving the cleanup commits only in case they are not by the same author (so people can see the changes others made to their code).

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment