FreeCAD Renderer Workbench improvements

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
User avatar
furti
Posts: 343
Joined: Mon Nov 27, 2017 5:27 pm

FreeCAD Renderer Workbench improvements

Postby furti » Sun Sep 08, 2019 7:45 am

I created a pull request that refactors some things in the Renderer Workbench in preparation to incorporate textures from the ArchTexture workbench.

The pull request is here and should be the base for discussions if this goes in the right direction. https://github.com/FreeCAD/FreeCAD-render/pull/13

I create this thread because @Nocturnial told me that he did some similar refactorings in his fork of the Render Workbench.

The material changes are similar. Except he uses a dict and I created a RenderMaterial class instead.
I went for a dict before, but I like the class approach more. It has two advantages.
1. With a dict you have to write "material['color']" with a class you can write "material.color". I think the later is easier to read. But this might be personal taste.
2. A class can be extended in the future to have methods. E.g. all renderers for now have the same use for the color. Create a comma separated list of the RGB values. So this could easily be implemented as a "getColorString" method on the RenderMaterial. @Nocturnial solved it by using a DiffuseColor property that is already a string. I think this is less flexible because maybe in the future we have a renderer that needs another representation of colors. Having a list of numbers is easier to transform the having to split a string back into its separate parts.

yorik wrote: ping
Nocturnial wrote: ping
Nocturnial
Posts: 27
Joined: Thu Aug 15, 2019 9:42 am

Re: FreeCAD Renderer Workbench improvements

Postby Nocturnial » Sun Sep 08, 2019 11:54 am

I don't mind being pinged. But I really don't understand why you pinged me for comments after you've issued a PR instead of before.
User avatar
Kunda1
Posts: 8413
Joined: Thu Jan 05, 2017 9:03 pm

Re: FreeCAD Renderer Workbench improvements

Postby Kunda1 » Sun Sep 08, 2019 1:09 pm

Nocturnial wrote:
Sun Sep 08, 2019 11:54 am
I don't mind being pinged. But I really don't understand why you pinged me for comments after you've issued a PR instead of before.
Perhaps to review the code?
Want to contribute back to FC? Checkout:
#lowhangingfruit | Use the Source, Luke. | How to Help FreeCAD | How to report FC bugs and features
User avatar
furti
Posts: 343
Joined: Mon Nov 27, 2017 5:27 pm

Re: FreeCAD Renderer Workbench improvements

Postby furti » Sun Sep 08, 2019 1:23 pm

Nocturnial wrote:
Sun Sep 08, 2019 11:54 am
But I really don't understand why you pinged me for comments after you've issued a PR instead of before
As i wrote in the PR it is intented for discussion. Its easier to discuss actual code than simply talk about it. :)

Github makes it possible to write comments Into the code and stuff like this.
The PR must not be merged. It only needs to be reviewed. When this is the wrong direction we can still close the PR and create a new one.

The only thing I see that needs to be discussed is: should the material be a dict or a separate class. Aftetwards we can continue development separately without getting into conflict i think.
vocx
Posts: 5206
Joined: Thu Oct 18, 2018 9:18 pm

Re: FreeCAD Renderer Workbench improvements

Postby vocx » Sun Sep 08, 2019 4:15 pm

furti wrote:
Sun Sep 08, 2019 1:23 pm
...

The only thing I see that needs to be discussed is: should the material be a dict or a separate class. Aftetwards we can continue development separately without getting into conflict i think.

A material class sounds good the way you describe it.

And @Nocturnial, a pull request is not meant to be final. It can always be discussed and rejected.
Always add the important information to your posts if you need help. Also see Tutorials and Video tutorials.
To support the documentation effort, and code development, your donation is appreciated: liberapay.com/FreeCAD.
Nocturnial
Posts: 27
Joined: Thu Aug 15, 2019 9:42 am

Re: FreeCAD Renderer Workbench improvements

Postby Nocturnial » Sun Sep 08, 2019 10:15 pm

furti wrote:
Sun Sep 08, 2019 1:23 pm
The only thing I see that needs to be discussed is: should the material be a dict or a separate class. Aftetwards we can continue development separately without getting into conflict i think.
Of course it should be a class.
I'm currently using the dictionary from the freecad material directly because I'm not sure what the best way would be to implement it.

At the moment, I'm thinking about using a wrapper class around the material dictionary. It would contain helper functions like getFloat("blabla") getFloatInverted("...") getColorCommas("") getColorSpaces("") (Don't mind the names, I'll try to come up with something better).

Using a dictionary also means you can put anything you want in it and there's no central place where you can see what variables could be used. That's why each time I add a variable I put it in materials.md (https://github.com/justnope/FreeCAD-ren ... terials.md). The documentation needs to be written anyway but it becomes more important if we keep the dictionary approach. Materials.md at the moment is a template where I copy-pasted stuff from the internet.

Maybe when I implement more renderers and shaders, I'll change my mind and find another approach that's more suited.

The approach you're suggesting seems ok if you're using only a couple of parameters. If there are a lot of parameters, it feels like a lot of extra typing and code with the only benefit you'll be able to use str(material.roughness) instead of material.getFloat("Roughness")
User avatar
furti
Posts: 343
Joined: Mon Nov 27, 2017 5:27 pm

Re: FreeCAD Renderer Workbench improvements

Postby furti » Mon Sep 09, 2019 5:04 am

Having a dict to store the data in the class is also fine for me. But i would not use a "getFloat("xxx")" approach but instead add dedicated "getXxx()" methods like "getRoughness()" "getTextureFile()" and so on.

Because this gives a dedicated API. Otherwise i can do stuff like "getFloat("TextureFile")" and that makes not much sense :)
User avatar
furti
Posts: 343
Joined: Mon Nov 27, 2017 5:27 pm

Re: FreeCAD Renderer Workbench improvements

Postby furti » Mon Sep 09, 2019 5:54 am

Nocturnial wrote:
Sun Sep 08, 2019 10:15 pm
At the moment, I'm thinking about using a wrapper class around the material dictionary
Using the material dictionary directly will not work. Not everything is inside the core material.
Textures for example are handled by a external workbench. So we need a dedicated RenderMaterial that can have more properties than the native material.

Also roughness alone would not be enough. To get the most out of it, the renderer should support roughness maps also.

Putting everything into the core material might get confusing pretty soon i think.
Nocturnial
Posts: 27
Joined: Thu Aug 15, 2019 9:42 am

Re: FreeCAD Renderer Workbench improvements

Postby Nocturnial » Mon Sep 09, 2019 9:13 am

furti wrote:
Mon Sep 09, 2019 5:04 am
Having a dict to store the data in the class is also fine for me. But i would not use a "getFloat("xxx")" approach but instead add dedicated "getXxx()" methods like "getRoughness()" "getTextureFile()" and so on.

Because this gives a dedicated API. Otherwise i can do stuff like "getFloat("TextureFile")" and that makes not much sense :)
It would make things a lot easier for me if it were that simple.
Here are all the parameters:
appleseed bsdf
appleseed bssdf
luxcore materials
The blender parameters need to be looked up in the source.

I'm not going to write a separate function for all those parameters. If somebody else did this, I would use it of course.

hint: just because something has the same name in different shaders doesn't mean they are the same. This is actually my biggest challenge at the moment: figuring out which params have the same meaning across the different shaders and renderers.
User avatar
furti
Posts: 343
Joined: Mon Nov 27, 2017 5:27 pm

Re: FreeCAD Renderer Workbench improvements

Postby furti » Mon Sep 09, 2019 10:49 am

Sorry but i don't get the point of your post.
Nocturnial wrote:
Mon Sep 09, 2019 9:13 am
It would make things a lot easier for me if it were that simple.
What should be simple? And what is complicated right now?
Nocturnial wrote:
Mon Sep 09, 2019 9:13 am
I'm not going to write a separate function for all those parameters.
When i look at blenders principeld bsdf it has 17 inputs. Where a lot of them are not so relevant i think. So we will end up with 10 dedicated methods. That is not that much in my opinion.
Nocturnial wrote:
Mon Sep 09, 2019 9:13 am
just because something has the same name in different shaders doesn't mean they are the same
I know. When i talk about textures, i mean image files that are applied to the surface of an object.
But in povray "texture" has a totally different meaning. It maps to what we would call Material in FreeCAD.
But neverteless FreeCADs RenderMaterial should have a ImageTexture property and the povray renderer than knows how to transform the meaning of a FreeCAD texture to a povray texture.