ObjReader/MeshBuilder Issues

przem321 wrote at 2012-01-12 17:24:

Hi Mr. Objo,

well, first of all - congratulations, this lib a is very good effort. Eventually this makes WPF3D usable for more advanced projects. 

I am working on meshes and I have just switched to your lib for my new project. Thus, I play around with the ObjReader and MeshBuilder a little. Yesterday I provided you the minor fixes to the Opacity value to the ObjReader. Today I encountered further problems with importing general polygonal objects. After an investigation, I finally conclude that a couple of problems is caused by the null-default values of the MeshBuilder properties (MeshBuilder.cs, line: 198):

        public Vector3DCollection Normals
        {
            get
            {
                return this.normals ?? (this.normals = new Vector3DCollection());
            }
        }

This causes that your sanity checks in the Append(...) method may fail. In particular you check (MeshBuilder.cs, line 1818):

            if (this.normals != null && normalsToAppend == null)
            {
                throw new InvalidOperationException(SourceMeshNormalsShouldNotBeNull);
            }

The problem is, that if one has called the Normals property prior to this check (for what ever reason, maybe just to check if it is null) the filed normal is not null and the sanity check throws an exception here. This is not what we want I guess. Of course this also applies to other fields like positions and texcoords, though there will be hardly a mesh with a null-valued position array.

My suggested solution is to remove the null-default values from the properties - in fact I don't see a purpose for that here? I mean like that (MeshBuilder.cs, line: 198):

        public Vector3DCollection Normals
        {
            get
            {
                return this.normals;// ?? (this.normals = new Vector3DCollection());
            }
        }

This works fine with all types of meshes I have tested. But I might have caused some side-effects I am not aware of. Thus you should check it - if there is a specific reason why you set the default-values with the ?? operator than my solution might be wrong. Otherwise, I suggest to throw away the ??-defaults and to check normals, and texcoords for null in order to determine if the mesh uses them or not. This makes it all more controllable I guess, but its up to you. I will send you the MeshBuilder.cs as well as some further minor fixes in the ObjReader.cs per bug-report. 

Cheers

Przem

PS. I have also created half-edge based mesh data structures, so once my stuff is running properly, maybe you will be interested in replacing the (very) limited Mesh3D class. 

objo wrote at 2012-01-12 21:02:

hi Przem, you are right - the getter should simply return the this.normals field. Thank you for reviewing the code! I'll apply the patch tomorrow.

A half-edge or winged-edge mesh representation would be very interesting!! Great if you could submit an implementation and a tiny example how to use it! 

przem321 wrote at 2012-01-13 17:47:

Thanks for reply.

I have some further questions/suggestions. My first is the reason you are using the Collection<T> class for collection the Groups in the ObjReader (line 53). Wouldn't it be better to expose a IList<T> interface and allocate a List<T> internally, since the latter one is better optimized for speed than Collection<T>? Well, this is a minor issue, since ObjReader is not real-time critical.

The second issue is that members exposed by IModelReader are not very useful. While, true, it is fine to read a file and obtain a ready Model3DGroup, this does nos cover all the cases where you would like to edit the model. Furthermore, the rule of thumb is to expose as general as possible types, which than can still be typed more strongly. In the case of your lib the MeshBuilder class seems to me to be the most general one. Thus I would suggest the IModelReader  to deliver MeshBuilder or IList<MeshBuilder> results, which than can be very easily converted to whatever the user wishes. 

Third, I have a remark regarding the MeshBuilder: while this class is very good, I am afraid that it gets a little bit overcrowded. For example, I don't think that it is a proper place to implement SDS - this is a case for Mesh3D or some own class. I also think that all the factoring methods should be implemented as static methods such that they deliver distinct MeshBuilder which than can be appended to each other. Static methods make it easier to keep independence and also maintenance. Maybe it makes sense to extract some interface of the MeshBuilder? To sum up: I really like this class and I think this is the best for geometry exchange. 

Finally, I would suggest to use as general types as possible. For example the ScreenSpaceVisual3D class expects its points as a Points3DCollection, but internally it uses the collection just as a container. Therefore there is no need to use a Points3DCollection (which is only good for renderable MeshGeomerty3D objects), I would suggest to use IList<Point3D>, such that one can provide any IList of 3d points, in cases one holds the points in other containers. Otherwise one has to copy the points to a Point3DCollection which is not used anyway.

Well, these I just some thoughts. I must say that I am not a code-guru, I am more into maths and geometry, and I only code in order to get my geometry projects running (unfortunately, MATLAB is too limited for interactive 3d modeling). And I have to say again that this framework is a great effort and it is fun using it!

Cheers

Przem

objo wrote at 2012-01-13 20:17:

#1: Agree on this. I also changed in other classes that used Collection<T>. I remember having read this reply: http://stackoverflow.com/questions/271710/collectiont-versus-listt-what-should-you-use-on-your-interfaces, but I need to understand when to use Collections and Lists better..

#2: What if the imported model contains a hierarchy of meshes and transforms? And materials. Then I think the hierarchic Model3D structure is needed. The import classes could be more general (even platform independent), but it has not been the goal for the library (so far..) (I have simply used the WPF 3D classes where possible)

#3: Agree on this, the builder should only contain the simple mesh operations (no questionable/experimental algorithms...) I like to use the MeshBuilder to combine several objects using the same material into the same geometry (for performance), then I guess allocating a mesh for each object and finally appending all these meshes will cost more (should profile this). I think the MeshBuilder should work like the System.Text.StringBuilder, i.e. not as static factory methods. Good performance for large/composite meshes is the goal here.

#4: Agree, I am changing this!

Thanks for great comments! 

przem321 wrote at 2012-01-13 20:42:

Thant for reply.

#1: According to http://blogs.msdn.com/b/codeanalysis/archive/2006/04/27/585476.aspx one should use the Collection<T> if one wants to override some methods. This is usually not the case when one just wants to store the elements. 

#2: Well, on a one hand you are right. I thought a IList<MeshBuilder> might be an option. But this does not contain materials, etc. Maybe this is fine like it is. Or maybe one should use the Wavefront structure (Group,Material, etc.) in order to keep the object (for all types of inputs) after loading and than, on demand one might create the Model3DGroup. The problem I have with the current solution is that sometimes one does not want to obtain a Model3DGroup after loading. With the current solution in such a case one needs to convert the Model3DGroup back to whatever one needs. Or just to use the ObjReader directly without the IModelReader interface in order to get access to the Groups and Materials. Since the OBJ format is a well established exchange-format, why not to keep its structure internally as a "raw" model representation? Well, this is just a suggestion. But let me think a little on it, maybe I will come up with a better one is few days.

#3: I just thought each Primitive would be appended like the current Append(...) function. But you are right this would cause unnecessary overhead. Its fine like it is.

objo wrote at 2012-01-13 21:03:

#2: Yes, implementing the domain model of the Wavefront structure would be a good idea. Then create the WPF3D model on request.

Another issue with the obj reader is the handling of smoothing groups. It is currently flat shading everything.

przem321 wrote at 2012-01-26 19:15:

Hi,

just wanted to report a bug: in the current version of the ObjReader.cs, line 378, Polygone3D will not accept a List<Point3D> in the constructor. The project will not compile. I fixed it locally, but just wanted to let you know. 

objo wrote at 2012-01-26 19:41:

are you sure your Polygon3D.cs file is updated? There should be a constructor Polygon3D(IList<Point3D> pts) there.

bcblanka wrote at 2012-02-01 12:30:

Hi,

First of all great work on the Helix toolkit; we are using it in a 3D modelling application and we would have been far behind on development without this toolkit. I was recently working with importing 3D models from OBJ files (ObjReader) and I saw that the material applied on a 3D surface isn't always visible. These are my findings:

In the ObjReader.cs, GetMaterial() method where you are creating the brush for the 3D object, the opacity is set to: "Opacity = 1.0 - this.Dissolved" for both color or image brushes. This is what I read about OBJ materias:

"d factor 
Specifies the dissolve for the current material. 
factor - is the amount this material dissolves into the background. A factor of 1.0 is fully opaque. This is the default when a new material is created. A factor of 0.0 is fully dissolved (completely transparent). " http://www.fileformat.info/format/material/

So when an object has a dissolve factor of 1 (fully opaque) the GetMaterial() method will create a completely transparent material for it.

My fix for this issue was setting a default value of 1 for the Dissolved property, and setting the" Opacity = this.Dissolved" for the brush. It seems to be working, but I am not too familiar with obj files so this might be wrong or breaking other functionalities of obj reader.

Thanks for the great toolkit!

Blanka

objo wrote at 2012-02-01 18:53:

thanks for the bug report - this was the original implementation, but it was changed in #74137. I am reverting back to the original implementation now: Opacity = dissolve value. Also setting default value to 1.0. przem321, do you agree?

przem321 wrote at 2012-02-02 19:06:

Well, in my case this is exactly the opposite. I have used models converted in Deep Exploration and all models where invisible with Opacity = Dissolved. By inverting it as mentioned above, the models were fine. But according to the specification referenced above Dissolved should refer to Opacity, thus is seems that bcblanka is right. 

I will try to figure out what is the actual problem with my models and post it here than. 

bcblanka wrote at 2012-02-03 07:26:

Hi,

The code: "Opacity = 1.0 - this.Dissolved" worked fine when the dissolve factor was not specified in the *.mtl file. Maybe that is the case for your models, przem321. However, if a dissolve value is specified in the *.mtl file, for example 1 for opaque, the opacity will be 0 (Opacity = 1.0 - this.Dissolved; Opacity = 1-1).

objo wrote at 2012-02-03 08:41:

I changed the default value for Dissolved to 1.0

przem321 wrote at 2012-02-03 15:46:

bcblanka wrote:

The code: "Opacity = 1.0 - this.Dissolved" worked fine when the dissolve factor was not specified in the *.mtl file. Maybe that is the case for your models,

This is clear. The problem was that the code: "Opacity = Dissolved" didn't worked, but it did other way, this is the reason I've changed it. It appeared that Dissolved applies to the model's transparency, but I haven't checked the spec though. 

Now, I guess there must be some other reason, like that the models are skewed up. I'll try to check this in a free moment. Thanks for clarification.