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

ShapeFrame and ShapeNode #608

Merged
merged 52 commits into from Mar 18, 2016
Merged

ShapeFrame and ShapeNode #608

merged 52 commits into from Mar 18, 2016

Conversation

@jslee02
Copy link
Member

jslee02 commented Feb 12, 2016

This is a work-in-progress pull request for implementing ShapeFrame/ShapeNode as discussed in #394.

@jslee02 jslee02 added this to the DART 6.0.0 milestone Feb 12, 2016
const ShapePtr& shape, const std::string& name = "ShapeNode");

/// Create an ShapeNode with the specified name
ShapeNode* createShapeNode(const ShapePtr& shape, const char* name);

This comment has been minimized.

@mkoval

mkoval Feb 12, 2016 Collaborator

I don't think it is necessary to have both a const char* overload and a const std::string& overload. Won't the const char* implicitly convert to an std::string?

This comment has been minimized.

@mxgrey

mxgrey Feb 12, 2016 Member

I'm not sure about this particular case, but I've seen cases where having an overloaded function where one of the overloads takes a std::string, the compiler won't always implicitly convert it when you pass in a const char*. I've especially seen this for templated functions, which the next one down is. (I don't know about this specific case, since @jslee02 wrote it, but I'm guessing it's a similar situation here.)

This comment has been minimized.

@jslee02

jslee02 Feb 13, 2016 Author Member

As @mxgrey said, the implicit converting sometimes didn't work. I'm still not sure exactly when is the case. I just tested this function and BodyNode::createEndEffector(const char*), and the implicit converting worked for both of them. So we can remove these functions.

Edit: My test went wrong. The conversion doesn't work so we need the overloaded functions. This might be the case of templated function case.

This comment has been minimized.

@mxgrey

mxgrey Feb 13, 2016 Member

I think the way it works is that it will attempt to use a templated function instead of performing an implicit conversion, but then the templated function fails because it wasn't meant for the case of a const char*.

This comment has been minimized.

@jslee02

jslee02 Feb 13, 2016 Author Member

I see. The process of type deduction for templated function doesn't check if the passed-in parameter type can be implicitly converted to one of possible types. Good to know!

/// allow it to render again.
void setHidden(bool _hide=true);
/// Notify that the color (rgba) of this shape has updated
virtual void notifyColorUpdate(const Eigen::Vector4d& color);

This comment has been minimized.

@mxgrey

mxgrey Feb 13, 2016 Member

Do shapes have Color anymore? I thought Shapes were now purely geometric information, and that color information would be in the VisualAddon of the ShapeNode. Or is this just WIP code that's going to be removed later?

This comment has been minimized.

@jslee02

jslee02 Feb 13, 2016 Author Member

I thought so too, but I realized MeshShape has color properties (colors for the vertices) in it. This function gets called when the color of VisualAddon is changed so that MeshShape can handle it. By default, it does nothing for other shapes. This function is a compromised solution, so I'm open to any better way for this case.

This comment has been minimized.

@mxgrey

mxgrey Feb 13, 2016 Member

There has been some discussion about the possibility of MeshShape being changed into something that only holds geometric data. If we do that, then we could have it so that the texture and color data gets automatically put into the VisualAddon as it's parsed with assimp. I think this would be also be nice for moving away from assimp being a core dependency (instead it would only be an io dependency).

That said, I don't know what the timeline should be for implementing something like that. We could probably hold off on it until version 7.1.

This comment has been minimized.

@jslee02

jslee02 Feb 13, 2016 Author Member

Yeah, the change sounds good to me. If I remember correctly, there is ongoing work of Mesh class (for moving away from assimp) by @mkoval. Once it's done we can remove this function. Maybe a note for this here would be good.

This comment has been minimized.

@jslee02

jslee02 Feb 13, 2016 Author Member

Okay, I didn't remember correctly. 😭 @mklingen was working on it at here. Ref: #453.

This comment has been minimized.

@mkoval

mkoval Feb 14, 2016 Collaborator

I very much do not want DART in the business of defining a canonical set of material properties. I am cautiously optimistic about @PyryM's suggestion:

I think materials should just be key/value stores that map string keys to [vec4 | texture | string] values.

It is straightforward to allow the user to store arbitrary vertex properties. We could simply allow the user to store a std::vector<T> with size() equal to the number of vertices. I am less sure about providing an API to store textures. Hopefully @PyryM can clarify a few points:

  • Assimp supports spherical, cylindrical, and uv mapping. Are all three used in practice?
  • I also found some references to uvw-mapping. How does this differ from uv mapping?
  • Are uv-coordinates stored as separate vertex properties or attached to the image asset?
  • How are uv-coordinates associated with the corresponding assert?
  • Are all textures 24-bit RGBA? Is it common to use a different bit depth or number of channels?

My only other concern is that VisualAddon is attached to ShapeFrame, not Shape. These properties only apply to MeshShape and are intimately related to the mesh, e.g. arrays of vertex properties must have the same length as the number of vertices in the mesh.

Alternatively, we could take the simpler route I suggested in #453:

MeshNode should contain the bare minimum amount of information necessary to perform collision checking and a dynamical simulation. Additionally, DART should provide an Uri to the original resource that can be loaded for visualization. The user is responsible for loading any additional information, e.g. textures and material properties, that is necessary only for visualization.

This has two downsides. First, every mesh is loaded from disk twice: once for collision detection and once for visualization. Second, we cannot programmatically construct a textured or vertex-colored mesh. This latter limitation bothers me more than the former - @mklingen and I independently ran into this in our research when rendering a color map.

This comment has been minimized.

@psigen

psigen Feb 14, 2016 Collaborator

Regarding the method in #453:

The user is responsible for loading any additional information, e.g. textures and material properties, that is necessary only for visualization.

Consider loading an object from a file URI but then changing the object on disk (I was trying to adjust some objects in a scene while I had it open in rviz). There is zero guarantee that each loading operation will have the same result, especially if assets are lazily-loaded, which can lead to out-of-sync visuals or even more pathological consistency issues.

This might even specifically be intentional in cases where an object is being dynamically updated, such as serving a generated model that results from perception or some other process at an http URI.

We also currently have to run an HTTP proxy server and rewrite URIs to implement dart_rviz, because we have the problem that URIs that are resolvable in the originating DART process aren't necessarily resolvable in the scope of the visualization (because it can be run over a network).

So, while it is very semantically clean, it does have some significant downsides.

This comment has been minimized.

@PyryM

PyryM Feb 14, 2016

Assimp supports spherical, cylindrical, and uv mapping. Are all three used in practice?

Spherical and cylindrical are ways of automatically assigning uv coordinates. I wouldn't bother trying to directly support them since any 3d editing program can compute these mappings and export them as regular uv coordinates.

Are uv-coordinates stored as separate vertex properties or attached to the image asset?
I also found some references to uvw-mapping. How does this differ from uv mapping?

Texture coordinates are vertex attributes, they're part of the mesh data like vertex positions and normals. Typical texture mapping, to simplify a lot, works like color = my_texture[u,v]. You can have 3d textures (a 3d array compared to a 2d array for a normal texture) in which case color = my_texture[u,v,w]. But the names and meanings of all the vertex attributes are just conventions, and shaders are free to interpret the attributes however they want.

Are all textures 24-bit RGBA? Is it common to use a different bit depth or number of channels?

32 bit RGBA8/BGRA8 is the most common, and probably the only format that's likely to be encountered in this application. It might be worth supporting float textures (R32F or RGBA32F) to make it convenient to visualize procedurally generated data without having to squash or encode it into RGBA8.

This comment has been minimized.

@mkoval

mkoval Feb 17, 2016 Collaborator

This discussion moved to #612. In the meantime, should we remove this function?

This comment has been minimized.

@jslee02

jslee02 Feb 17, 2016 Author Member

I think we should keep this function as far as MeshShape contains color data and we want to use and update it.

/// Create an ShapeNode with the specified name and addons
template <class... Addons>
ShapeNode* createShapeNode(const ShapePtr& shape,
const std::string& name = "ShapeNode");

This comment has been minimized.

@mkoval

mkoval Feb 13, 2016 Collaborator

I'm not sure if this overload is necessary. It only seems helpful in the case where you want to create a ShapeNode with several AddOns, but you don't need to set any properties on them. Any other case would be better served by adding a getOrCreate function to `AddonManager, e.g.:

ShapeNode* node = bodyNode->createShapeNode();
node->getOrCreate<VisualAddon>()->mColor = Orange();

This comment has been minimized.

@jslee02

jslee02 Feb 16, 2016 Author Member

This function was added exactly for the case you mentioned. One might want to create a ShapeNode with several addons as soon as it gets created. Here is one example case. Without this function, we need multiple lines for this like:

auto shapeNode = bodyNode->createShapeNode(shape);
shapeNode->createVisualAddon();
shapeNode->createCollisionAddon();
shapeNode->createDynamicsAddon();

I don't object to remove this function, but this would be useful for the above use case.

This comment has been minimized.

@mxgrey

mxgrey Feb 17, 2016 Member

I like the function, although I wonder if we can have it accept Properties arguments for each Addon which will use their default constructor if one is not provided. I can't think of a way to do that at the moment, but I suspect it may be possible with enough cleverness.

This comment has been minimized.

@mkoval

mkoval Feb 17, 2016 Collaborator

This is only useful if you want to create the Addon, but not set any properties on it. Is this something that we expect to happen frequently?

shapeNodes[i] = getShapeNode(i);

return shapeNodes;
}

This comment has been minimized.

@mkoval

mkoval Feb 13, 2016 Collaborator

Can we return a const std::vector<ShapeNode*>& here, like we do in most of the other non-const functions that return vectors?

This comment has been minimized.

@jslee02

jslee02 Feb 16, 2016 Author Member

👍 Also, I'm adding const version that returns std::vector<ShapeNode*>&.

This comment has been minimized.

@mkoval

mkoval Feb 17, 2016 Collaborator

This implementation is dangerous. One std::vector<ShapeNode*> be used for all BodyNode instances.

I also don't see the point of this change. Returning the vector by value should be efficient because of return value optimization or, in the worst case, move semantics. The only advantage of returning a const reference is to avoid creating a new vector in the first place.

If that is not possible, I suggest going back to returning by value.

This comment has been minimized.

@jslee02

jslee02 Feb 17, 2016 Author Member

This implementation is dangerous. One std::vector<ShapeNode*> be used for all BodyNode instances.

True.

The only advantage of returning a const reference is to avoid creating a new vector in the first place.

I thought returning the vector by value would create the vector not only the first time but also whenever it gets called. Is it incorrect?

It seems we need an efficient way to get the list of nodes from a node manager. For now, we should return it by value or have the vector for particular nodes in a derived class to return it as a const reference. Or, we just use getNumNode/getNode(index) fashion. I created an issue about this #613, but that wouldn't for this pull request at least. Once we found a way, it would be good to have getNodes<NodeType>() function in the node manager level.

I think making this function to return vector by value would be fine for now.

This comment has been minimized.

@mxgrey

mxgrey Feb 17, 2016 Member

I thought returning the vector by value would create the vector not only the first time but also whenever it gets called. Is it incorrect?

If it returns by value, it will potentially have to allocate memory once each time it gets called, unless it can avoid that by using return value optimization. But one memory allocation is the worst possible case thanks to move semantics.

The danger of this static variable is that users are likely to catch the return of this function in a const-reference, but the contents of the vector that it's referring to would change as soon as the function gets called on a different BodyNode instance.

This comment has been minimized.

@jslee02

jslee02 Feb 17, 2016 Author Member

My concern was this case:

auto shapeNodes1 = bodyNode->getShapeNodes();  // vector gets created
// change number of shape nodes of bodyNode
auto shapeNodes2 = bodyNode->getShapeNodes();  // another vector gets created

If we have the vector as a member variable of BodyNode, then the vector is created once when BodyNode instance is created no matter whether the num of shape nodes is changed after. Probably we would be able to avoid memory allocation if the new number of shape nodes is less than the allocated memory size (of course it depends on the implementation of vector).

This comment has been minimized.

@mxgrey

mxgrey Feb 17, 2016 Member

Right, so far I've addressed this by having a cache of std::vector<const T> to complement the ordinarily used std::vector<T>. This was the best solution I was able to come up with at the time, but admittedly it's doubling memory consumption in some places. I'll be very interested to look over your wrapper class idea as soon as I get the chance.

void DynamicsShapeFrameAddonUpdate(DynamicsAddon* /*addon*/)
{
// Do nothing
}

This comment has been minimized.

@mkoval

mkoval Feb 13, 2016 Collaborator

What are these empty functions for?

This comment has been minimized.

@jslee02

jslee02 Feb 16, 2016 Author Member

These functions get called when the addon properties are changed. I created them as placeholders expecting to add something there, but haven't found the use yet. I'll remove these and add back when necessary.

//==============================================================================
void VisualAddon::show()
{
setHidden(true);

This comment has been minimized.

@mkoval

mkoval Feb 13, 2016 Collaborator

show() and hide() appear to be reversed - unless setHidden has very unintuitive behavior. 😄

This comment has been minimized.

@jslee02

jslee02 Feb 16, 2016 Author Member

setHidden seemed a very complicated function to me. 😂

{
copy(other);
return *this;
}

This comment has been minimized.

@mkoval

mkoval Feb 13, 2016 Collaborator

I'm not sure if it's a good idea to overload the assignment operator on this class. This is a pure virtual class, so all sorts of bad things could happen if you you try to assign two different ShapeFrame subclasses.

This comment has been minimized.

@mxgrey

mxgrey Feb 13, 2016 Member

Yeah, I'm definitely inclined to agree on this. If anything, we should delete the assignment operator for ShapeFrame and leave it to SimpleFrame and ShapeNode to write it.

This comment has been minimized.

@jslee02

jslee02 Feb 16, 2016 Author Member

👍

}

//==============================================================================
DynamicsAddon* ShapeFrame::getDynamicsShapeNode(const bool createIfNull)

This comment has been minimized.

@mkoval

mkoval Feb 13, 2016 Collaborator

This should be named getDynamicsAddon to be consistent with the two above methods.

This comment has been minimized.

@jslee02

jslee02 Feb 16, 2016 Author Member

👍

return createDynamicsAddon();

return dynamicsData;
}

This comment has been minimized.

@mkoval

mkoval Feb 13, 2016 Collaborator

It would be nicer to replace these three functions with a generic createOrGet<AddonT> template function on AddonManager. Is there a strong reason to define them here?

This comment has been minimized.

@mxgrey

mxgrey Feb 13, 2016 Member

I'm guilty for starting the trend of allowing a bool to decide whether the get<AddonT>(bool createIfNull = false) call also creates an instance. I think that's nice for programmatically being able to decide whether or not to automatically generate one.

That said, I do think it would be good to also have a getOrCreate<AddonT>() function. I would like both to live side-by-side unless there's a strong objection to the get<AddonT>(bool createIfNull) version.

This comment has been minimized.

@mkoval

mkoval Feb 14, 2016 Collaborator

I am fine with get<Addon>(bool) being the only API. I am actually suggesting that we remove these three methods entirely, because they are identical to get<T>(true). What's the advantage of getVisualAddon() over get<VisualAddon>(true)?

This comment has been minimized.

@mxgrey

mxgrey Feb 14, 2016 Member

For me, the motivation of having these functions baked in is to use the API to make it perfectly clear to users what kind of Addons are specialized for the object.

That said, I think the functions should do nothing but wrap the templated versions. I made a macro for creating such functions, so I'd suggest using that here.

This comment has been minimized.

@mkoval

mkoval Feb 17, 2016 Collaborator

👍 to use a macro to create this functions. It avoids silly typos in duplicate code.

This comment has been minimized.

@mxgrey

mxgrey Mar 4, 2016 Member

Should we replace these with macros before or after the pull request goes through?

This comment has been minimized.

@jslee02

jslee02 Mar 4, 2016 Author Member

Let's do it in this pull request.

Besides on that, would it be fine with you to change the macro style adding more lines like:

#define DART_BAKE_SPECIALIZED_ADDON_IRREGULAR( TypeName, AddonName ) \
  inline bool has ## AddonName () const                              \
  {                                                                  \
    return this->template has<TypeName>();                           \
  }                                                                  \

?

void removeAllShapeNodes();

/// Add a collision Shape into the BodyNode
void addCollisionShapeNode(const ShapePtr& shape);

This comment has been minimized.

@mxgrey

mxgrey Mar 9, 2016 Member

It looks like this function has been left undefined. I'm not exactly sure how we should approach defining it. Should the ShapeNode be given default properties? I'm inclined to say that we don't really need this function, since we already have the templated version of createShapeNode that allows the collision addon to be created simultaneously.

This comment has been minimized.

@jslee02

jslee02 Mar 11, 2016 Author Member

Yeah, I also intended to remove this function but forgot to remove the declaration. Removing.

void addCollisionShapeNode(const ShapePtr& shape);

/// Remove a collision Shape from this BodyNode
void removeCollisionShapeNode(const ShapePtr& shape);

This comment has been minimized.

@mxgrey

mxgrey Mar 9, 2016 Member

Like above, this function is currently undefined. Would it remove all ShapeNodes that contain this particular shape? I'm inclined to think that this function shouldn't exist either.

@mxgrey
Copy link
Member

mxgrey commented Mar 9, 2016

I've finished reviewing. All of my comments which are relevant to this pull request have been addressed. I've also gone ahead and made some changes to the API for accessing and creating ShapeNodes. In particular, I've added With to all functions that operate on a subset of ShapeNodes that contain a particular Addon type. I've also removed the AddonAdder struct and replaced it with two void-returning variadic functions which are now located in common/AddonManager.h.

I only have one remaining concern: It seems that soft bodies are very very broken right now. If we're okay with soft bodies being broken on master, then I'm fine with merging in this pull request. I do think we should try to get them at least looking believable before the major release, though.

@jslee02
Copy link
Member Author

jslee02 commented Mar 10, 2016

Could you take a look at osgDart as well? I made it just work (I believe ) but there are some stopgaps. For example, EntityNode is replaced by ShapeFrameNode but WorldNode is still using EntityNode.

I'm looking at soft bodies now.

@mxgrey
Copy link
Member

mxgrey commented Mar 10, 2016

The drag and drop features in osgDart seem to be working fine. Unfortunately, the diff for those files isn't showing up for me on Github. If anyone can recommend a good git diff viewing desktop application, I would appreciate it. Otherwise, I can comb through the raw code some time soon.

@psigen
Copy link
Collaborator

psigen commented Mar 10, 2016

On Ubuntu, I generally use meld.

@mxgrey
Copy link
Member

mxgrey commented Mar 10, 2016

I use meld for merging, but I don't like looking back and forth from side to side when it comes to comparing two files. I really like the linear way that Github lays it out. The standard console tool lays it out similarly, but isn't very pretty. I probably won't be able to find what I want, so I'll just have to settle for meld or the console. It would be nice if Github offered an offline version of their viewer.

jslee02 added 5 commits Mar 10, 2016
…rement/decrement reference count on its skeleton. This is necessary to store a valid WeakShapeNodePtr to SoftBodyNode during the SoftBodyNode is being created.
@jslee02
Copy link
Member Author

jslee02 commented Mar 11, 2016

It would be nice if Github offered an offline version of their viewer.
👍

I created two dummy branches (with and without osgDart changes) to help reviewing osgDart here. It doesn't include commit-wise changes of the original history, but you can see the final diff.

Also, I resolved the instability of the soft bodies in the last commits.

@psigen
Copy link
Collaborator

psigen commented Mar 12, 2016

@mxgrey you can possibly try the vertical diff mode in vimdiff?
http://stackoverflow.com/a/244766

I actually find it hard to use the unified vertical diff, because it makes it really tricky to compare large block diffs, but I think several difftools have flags to go into that mode.

@mxgrey

This comment has been minimized.

Copy link
Member

mxgrey commented on osgDart/DefaultEventHandler.h in 4c99fb8 Mar 14, 2016

I know this originally used to be an Entity*, but is there any reason not to use a ShapeFrame* now that only ShapeFrames are able to contain shapes? I feel that using ShapeFrame* now would be an overall enhancement without any downside.

This comment has been minimized.

Copy link
Member Author

jslee02 replied Mar 14, 2016

If I understand correctly, ownerEntity is for a pointer to the target object of drag-and-drop. If we restrict this variable to ShapeFrame* then we wouldn't be able to have drag-and-drop class whose target object doesn't inherit ShapeFrame. However, we have DragAndDrop whose target object is BodyNode which doesn't inherit ShapeFrame.

This comment has been minimized.

Copy link
Member

mxgrey replied Mar 14, 2016

I'm pretty sure it's a pointer to the Entity that has been clicked on, which (from now on) should always be a ShapeFrame, because only ShapeFrames get rendered now. At least that's what I had intended the PickInfo to mean. I can take a closer look at how the PickInfo is getting created to check whether or not this is the case.

This comment has been minimized.

Copy link
Member Author

jslee02 replied Mar 14, 2016

I assumed that DragAndDrop::mEntity is changed to ShapeFrame*. Changing the type of ownerEntity to ShapeFrame* doesn't have any technical problem if we're fine with comparing Entity* and ShapeFrame* for picking.

mxgrey added 2 commits Mar 14, 2016
@mxgrey
Copy link
Member

mxgrey commented Mar 15, 2016

I've pushed some fixes to the way Addons are cloned, and I've restructured osgDart. Soft bodies seem to be working again after @jslee02 patched it.

I think this pull request is good to merge now 👍

@jslee02
Copy link
Member Author

jslee02 commented Mar 15, 2016

👍 looks good to me too. Will merge this once #607 is merged.

jslee02 added 2 commits Mar 18, 2016
Conflicts:
	dart/dynamics/BodyNode.h
	dart/dynamics/EndEffector.h
	dart/dynamics/Skeleton.h
	dart/dynamics/detail/SpecializedNodeManager.h
jslee02 added a commit that referenced this pull request Mar 18, 2016
[WIP] ShapeFrame and ShapeNode
@jslee02 jslee02 merged commit 26db3c7 into master Mar 18, 2016
2 of 4 checks passed
2 of 4 checks passed
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jslee02 jslee02 deleted the shapenode branch Mar 23, 2016
@jslee02 jslee02 changed the title [WIP] ShapeFrame and ShapeNode ShapeFrame and ShapeNode Mar 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.