jscad / OpenJSCAD.org

JSCAD is an open source set of modular, browser and command line tools for creating parametric 2D and 3D designs with JavaScript code. It provides a quick, precise and reproducible method for generating 3D models, and is especially useful for 3D printing applications.
https://openjscad.xyz/
MIT License
2.58k stars 505 forks source link

x3d serializer: Material colors should be RGB, not RGBA #1231

Closed andreasplesch closed 1 year ago

andreasplesch commented 1 year ago

Expected Behavior

x3d serializer should generate

<Appearance>
        <Material diffuseColor="0 1 0" emissiveColor="0 1 0" transparency="0.2"/>
</Appearance

Actual Behavior

<Appearance>
        <Material diffuseColor="0 1 0 0.8" emissiveColor="0 1 0 0.8"/>
</Appearance>

X3D Material uses RGB format with a separate transparency field.

Steps to Reproduce the Problem

  1. load basic colors example on https://www.openjscad.xyz/
  2. export to X3D file
  3. find Material node

Specifications

X3D: Material SFColor fields, SFColor is RGB https://www.web3d.org/documents/specifications/19775-1/V3.3/Part01/components/shape.html#Material https://www.web3d.org/documents/specifications/19775-1/V3.3/Part01/fieldsDef.html#SFColorAndMFColor

andreasplesch commented 1 year ago

https://github.com/jscad/OpenJSCAD.org/blob/2b62d762056aadca75006ac4b1eebd08bd7addaa/packages/io/x3d-serializer/src/index.js#L173-L178

has the incorrect RGBA generation.

Potential solution assuming object.color is always a RGBA array:

const convertAppearance = (object, options) => {
   const colorRGB = object.color.slice(0, 3)
   const diffuseColor = colorRGB.join(' ') 
   const emissiveColor = colorRGB.join(' ')
   const transparency = 1.0 - object.color[3]
   return ['Appearance', ['Material', {diffuseColor, emissiveColor, transparency}]] 
 } 

x-ite has a simple x3d viewer for testing:

https://create3000.github.io/x_ite/playground/

Using both the diffuseColor and emissiveColor fields probably leads to very bright colors. This may be intended. But another option is use only emissive, and a default, gray diffuse, Or only diffuse.

andreasplesch commented 1 year ago

https://github.com/jscad/OpenJSCAD.org/blob/2b62d762056aadca75006ac4b1eebd08bd7addaa/packages/modeling/src/colors/colorize.js#L48

object.color can be assumed to be RGBA.

Unless somebody does not use the API and colorize() and just manually adds the .color property to an object. But this would then mean full responsibility. Is that common although not supported ? Probably not.

Perhaps add a guard:

const convertAppearance = (object, options) => {
   let color = object.color
   if (color.length !== 4) color = options.color
   const colorRGB = color.slice(0, 3)
   const diffuseColor = colorRGB.join(' ') 
   const emissiveColor = colorRGB.join(' ')
   const transparency = 1.0 - color[3]
   return ['Appearance', ['Material', {diffuseColor, emissiveColor, transparency}]] 
 } 

But should not be necessary.

andreasplesch commented 1 year ago

https://github.com/andreasplesch/OpenJSCAD.org/tree/x3d

has a branch with these changes and an augmented test. I will be happy to submit a PR.

z3dev commented 1 year ago

https://github.com/andreasplesch/OpenJSCAD.org/tree/x3d

has a branch with these changes and an augmented test. I will be happy to submit a PR.

@andreasplesch That would be super!

sorry for the late reply

andreasplesch commented 1 year ago

Ok !

I think it would be also preferable to just use Material emissiveColor, with default diffuseColor, as it would better align with perTriangle coloring using the Color node which essentially assigns emissive colors. It may also compare better with rendering by the default regl renderer.

Let me PR with just the RGB changes. Additional X3D serializer changes could then be added or go into another PR as appropriate.

z3dev commented 1 year ago

Ok !

I think it would be also preferable to just use Material emissiveColor, with default diffuseColor, as it would better align with perTriangle coloring using the Color node which essentially assigns emissive colors. It may also compare better with rendering by the default regl renderer.

Let me PR with just the RGB changes. Additional X3D serializer changes could then be added or go into another PR as appropriate.

@andreasplesch Great!

yes. please keep the changes small so we can easily review and comment.

looking forward t9 the pull request

andreasplesch commented 1 year ago

It is customary for X3D to omit fields which have default values. Since transparency will often be 0, the default, it may make sense to special case. It would add a few lines:

   let fields = { diffuseColor, emissiveColor }
   const transparency = 1.0 - object.color[3]
   if (transparency > 0) fields = { diffuseColor, emissiveColor, transparency }
   return ['Appearance', ['Material', fields]]

But on the other hand jscad tends to explicitly include alpha even if 1.0, I believe. Hm, I still think it would be nicer to omit transparency when not necessary.

andreasplesch commented 1 year ago

Just a note. It turns out using only diffuse is closest to the regl renderer:

image

regl: image

The regl renderer also uses a little bit of specular/shininess to focus the highlights.

z3dev commented 1 year ago

But on the other hand jscad tends to explicitly include alpha even if 1.0, I believe.

That's correct. All color values are RGBA. There's no need to check.

andreasplesch commented 1 year ago

Here is the x3d with a bit of shininess and facets for the red sphere which only requires adding a normalPerVertex="false" attribute:

image

https://gist.githack.com/andreasplesch/dc3595c5cf5198bedd7dbdb119a7a059/raw/acbd9a29c4618d82534fc0c8a6ae8317c22310cd/x3dom_jscad.html

https://gist.github.com/andreasplesch/dc3595c5cf5198bedd7dbdb119a7a059

This would be my preferred x3d color serialization. Perhaps it will make sense to add a 'smooth' option.

andreasplesch commented 1 year ago

continued in #1234