appleseedhq / appleseed

A modern open source rendering engine for animation and visual effects
https://appleseedhq.net/
MIT License
2.21k stars 331 forks source link

Properly transform normals in Alembic mesh file reader #323

Closed dictoon closed 9 years ago

Narann commented 10 years ago

The line: https://github.com/appleseedhq/appleseed/blob/master/src/appleseed/foundation/mesh/alembicmeshfilereader.cpp#L141

marius-avram commented 10 years ago

I'm going to have a look at this one.

dictoon commented 10 years ago

Excellent!

marius-avram commented 10 years ago

After I looked over the code some time I need to ask some questions. The transform which should be applied is the transform of the assembly_instance ? In that case, is the transform made when other meshes are loaded, for example .obj files and .binarymesh files ? From what I've seen they don't seem to have any transform applied. If they are needed for all kinds of meshes, then they could better be applied in the mesh builder functions.

A good question would be: how do I test this ? Because the appleseed.studio doesn't seem to have any way of applying a transform from the menus. Should I edit the project xml file by hand and notice if the loaded mesh changes somehow ?

Also there is a similar comment in the alembicmeshfilereader.cpp file for vertices. I suppose the bug applies to that to.

I hope this isn't a alembic file format specific problem and I looked in the wrong place until now.

dictoon commented 10 years ago

The transforms to apply are those internal to the Alembic file. Alembic files support transform hierarchies, and the "current" transform must be updated as the hierarchy is traversed.

You will need to refer to the Alembic docs: http://docs.alembic.io/. In particular this section is probably useful: http://docs.alembic.io/python/examples.html#accumulating-a-transform.

dictoon commented 10 years ago

Here is a scene that uses an Alembic file for geometry definition. The normals are not correctly transformed, leading to a mixture of front facing and back facing surfaces, and incorrect lighting (use the diagnostic modes to inspect the scene): http://appleseedhq.net/stuff/guerilla_bedroom.zip

marius-avram commented 10 years ago

I think I grasped the main idea and I tried to solve the bug. But I have a linking error when I try to instantiate some alembic classes. The problem is that the classes which cannot be found seems to be from openEXR. Here is the output of the make command:

Linking CXX executable ../../linux-gcc4/appleseed.cli/appleseed.cli
../../linux-gcc4/appleseed/libappleseed.so: undefined reference to `typeinfo for Iex::BaseExc'
../../linux-gcc4/appleseed/libappleseed.so: undefined reference to `Iex::BaseExc::BaseExc(char const*)'
../../linux-gcc4/appleseed/libappleseed.so: undefined reference to `Iex::BaseExc::what() const'
../../linux-gcc4/appleseed/libappleseed.so: undefined reference to `Iex::BaseExc::~BaseExc()'

But isn't appleseed usually linking with openEXR ? I would appreciate any helpful advice on how I can solve this.

This is my work in progress: https://github.com/marius-avram/appleseed/commit/1f15f3e1ea18c9177befe5b8b7bb591664d16b40

Problem appears when I'm instantiating:

IXform xform(object, kWrapExisting);
dictoon commented 10 years ago

But isn't appleseed usually linking with openEXR ? I would appreciate any helpful advice on how I can solve this.

Yes, appleseed already links against OpenEXR. In fact, it even already relies on Iex:BaseExc: https://github.com/appleseedhq/appleseed/blob/master/src/appleseed/foundation/image/exrimagefilewriter.cpp#L155

This is my work in progress: marius-avram@1f15f3ehttps://github.com/marius-avram/appleseed/commit/1f15f3e1ea18c9177befe5b8b7bb591664d16b40

Thanks!

Problem appears when I'm instantiating: IXform xform(object, kWrapExisting);

Do you confirm that if you simply remove this line, appleseed compiles and links correctly?

marius-avram commented 10 years ago

Yes, if I comment this three lines I can compile the code. IXform xform(object, kWrapExisting); XformSample xsample; xform.getSchema().get(xsample);

But if I uncomment one of them (preferably the first two) I receive the same linking error.

Now I noticed that the error is happening when the compile reaches appleseed.cli. Isn't this happening because of a misplaced included header ?

dictoon commented 10 years ago

Indeed, appleseed.cli does not link against OpenEXR. Now, I'm not sure why adding these lines to a .cpp file in appleseed itself (the shared library) would have any impact on linking appleseed.cli...

dictoon commented 10 years ago

Just to be clear: it's totally fine to link appleseed.cli against OpenEXR. We just need to understand why it's needed.

marius-avram commented 10 years ago

Can't seem to find the reason why this is happening. It looks like all what is missing is libIex. If I link it to appleseed with 'target_link_libraries' everything will compile fine with the entire change code uncommented. appleseed.cli is not the single one having the linking error. Same problem applies for:

appleseed.cli/CMakeLists.txt
appleseed.studio/CMakeLists.txt
tools/animatecamera/CMakeLists.txt
tools/convertmeshfile/CMakeLists.txt
tools/maketiledexr/CMakeLists.txt

Maybe there's something broken with my alembic installation ?

Even with my compilation hack the scene you posted doesn't seem to be changed with my piece of code. In what diagnostic mode is the problem more obvious ? I tried Shading Normals. Also when I'm loading the scene I get a few errors. Are they ok ? They are present even before my commit.

while loading mesh object "room_geo.Bedroom_NOAH_window02Shape": 1 polygonal face (out of 3,758) could not be triangulated and have been replaced by zero-area triangles.

Anyway I think I will put this on hold for a few days. Maybe I'll get some new ideas until then.

Until then I'm going to work on #43.

dictoon commented 10 years ago

Can't seem to find the reason why this is happening. It looks like all what is missing is libIex. If I link it to appleseed with 'target_link_libraries' everything will compile fine with the entire change code uncommented.

That's pretty strange: appleseed (I'm talking about the shared library, not about appleseed.cli or appleseed.studio) explicitely links against libIex:

https://github.com/appleseedhq/appleseed/blob/master/src/appleseed/CMakeLists.txt#L1660 https://github.com/appleseedhq/appleseed/blob/master/src/cmake/config/linux-gcc4.txt#L118 https://github.com/appleseedhq/appleseed/blob/master/src/cmake/config/linux-gcc4.txt#L131

appleseed.cli is not the single one having the linking error. Same problem applies for:

appleseed.cli/CMakeLists.txt appleseed.studio/CMakeLists.txt tools/animatecamera/CMakeLists.txt tools/convertmeshfile/CMakeLists.txt tools/maketiledexr/CMakeLists.txt

As I said in a previous comment, none of these (as far as I remember) link with OpenEXR at all. Did you try to simply add a link_against_openexr (appleseed) statement to these CMake files?

Even with my compilation hack the scene you posted doesn't seem to be changed with my piece of code. In what diagnostic mode is the problem more obvious ? I tried Shading Normals.

If you try the "Sides" diagnostic mode you should get something like this:

sides

Blue faces are front-facing, red faces are back-facing. I don't think this is how it's supposed to be on this scene.

The "Shading Normals" mode also displays a lot of discrepencies (flat walls should have the same normal everywhere, hence the same color):

shading_normals

Here is an actual render, as you can see the shading is completely broken (lots of artifacts everywhere, and also adjacent walls should have the same shade where they touch):

render

Playing with simpler Alembic files is maybe easier to check that your code is correct.

Also, don't hesitate to point us to your changes on GitHub, maybe someone can review them.

Also when I'm loading the scene I get a few errors. Are they ok ? They are present even before my commit.

while loading mesh object "room_geo.Bedroom_NOAH_window02Shape": 1 polygonal face (out of 3,758) could not be triangulated and have been replaced by zero-area triangles.

Yes, these are to be expected. They are not "normal" but they aren't out of the ordinary either. On large, complicated scenes it is common for our triangulator to find faces that cannot be triangulated.

Anyway I think I will put this on hold for a few days. Maybe I'll get some new ideas until then.

Until then I'm going to work on #43https://github.com/appleseedhq/appleseed/issues/43

No problem.

fewo commented 10 years ago

Even the simple cube example in the test/ alembic folder show the same problem, with no hierarchy. What is the difference between the "Shading Normals" and the "Original Shading Normals" ?

dictoon commented 10 years ago

You're right, the Alembic cube scene from the test suite shows the same problem.

About the shading modes:

Should we rename them to clear any ambiguity? Any suggestion?

fewo commented 10 years ago

No wanted to be sure and didn't take the time to look at the code. The funny thing is on the cube between the "shading normal" and the "orignal shading normal" the renders looks actually different where I think there's no normal mapping or bump mapping apply to it that why I was asking.

dictoon commented 10 years ago

The funny thing is on the cube between the "shading normal" and the "orignal shading normal" the renders looks actually different where I think there's no normal mapping or bump mapping apply to it that why I was asking.

Ok, this is weird :)

dictoon commented 9 years ago

We need to reimplement Alembic support completely, either as procedurals when they will be supported, or "fake procedurals" as Esteban suggested in private. Closing this issue.