FreeCAD Renderer Workbench improvements

[furti]

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.

ping

ping

[Nocturnial]

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.

[Kunda1]

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?

[furti]

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]

...

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.

[Nocturnial]

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")

[furti]

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

[furti]

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]

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.

[furti]

Sorry but i don't get the point of your post.

It would make things a lot easier for me if it were that simple.

What should be simple? And what is complicated right now?

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.

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.