Several functions are declared, but not defined

[mkoval]

These functions are declared, but not defined:

• most functions in render::Camera
• dart::constraint::BalanceConstraint::getPseudoInverseDamping()
• dart::renderer::Light::GetPosition()
• dart::renderer::Light::SetPosition()
• dart::renderer::Light::GetSpecular()
• dart::utils::convertFloatToDec
• dart::utils::minElem(std::vector<double, std::allocator<double> >&, int&)
• dart::utils::maxElem(std::vector<double, std::allocator<double> >&, int&)
• dart::utils::convertDecToFloat(char*)
• dart::constraint::ConstraintSolver::getNumConstraints() const

[mxgrey]

For convertFloatToDec, there is a definition given for ConvertFloatToDec (notice the uppercase "C"). It seems pretty obvious that they belong together. Neither of them appears to be getting used currently. Do we want to simply go with the lowercase version? I believe that would match the existing code style better.

[mxgrey]

For minElem and maxElem, these are currently unused, and there are already std functions for retrieving the min and max elements of an STL container. I would recommend deleting these. Would anyone object?

[mxgrey]

For ConstraintSolver::getNumConstraints, I'm not sure what we would really want this to return. Is there a specific use case for retrieving the number of constraints in a ConstraintSolver? I could imagine it returning either mManualConstraints.size() or mActiveConstraints.size(), depending on what the purpose of the function is (i.e. whether it's there to tell you how many constraints the user has added, or whether it's telling you how many constraints are currently being imposed by the simulation).

[jslee02]

For convertFloatToDec, we should go with the lowercase version according to our code style.

I don't object to removing minElem and maxElem in C3D.

I intended ConstraintSolver::getNumConstraints to return the number of constraints added by the user such as BallJointConstraint and WeldJointConstraint so that the use can access the constraints recursively within a for-loop with a getter that takes index of the constraint. However, I didn't even add the getter and couldn't find any usecase of it. So we can remove it.

[mxgrey]

Regarding the stuff in the renderer namespace, should we make them all pure virtual, or give them blank definitions? Or maybe give them definitions with a "Not available in your derived class" warnings?

[mxgrey]

Making the Camera functions pure virtual allows it to compile without any additional changes, so I think we should just go with that.

It looks like the Light class does not have any implementation whatsoever (unless it's doing a very good job of hiding). I wonder if we should just make it a struct and forget about the accessor functions.

[jslee02]

I'm fine with either of blank definition or making them as pure virtual since they are currently not used anyways, but warnings would be more gentle. 😄 They need to be removed or revised at some point though.

[mxgrey]

The Camera functions don't get used explicitly, but they get inherited and overridden by an OpenGL-specific camera class.

[jslee02]

Right. But the definitions of OpenGLCamera functions are also empty. Moreover Camera is not used at all. RenderInterface has Camera as its member but don't use it at all except for just returning the pointer. So there is no real use of Camera at least in our code base.

[mkoval]

I missed this in my original post, but the problem occurs on both convertFloatToDec and convertDecToFloat. It makes sense to switch to camel case to match the DART naming convention.

For minElem and maxElem, these are currently unused, and there are already std functions for retrieving the min and max elements of an STL container. I would recommend deleting these.

👍 I don't think these should exist.

I intended ConstraintSolver::getNumConstraints to return the number of constraints added by the user such as BallJointConstraint and WeldJointConstraint so that the use can access the constraints recursively within a for-loop with a getter that takes index of the constraint. However, I didn't even add the getter and couldn't find any usecase of it. So we can remove it.

👍 for removing this if we don't have a concrete use case for it.

Right. But the definitions of OpenGLCamera functions are also empty. Moreover Camera is not used at all. RenderInterface has Camera as its member but don't use it at all except for just returning the pointer. So there is no real use of Camera at least in our code base.

I would also prefer to remove Camera, since it has no functioning implementation and is not used anywhere else in the code base.

[jslee02]

I believe this is resolved by #558.