brochington / Akkad

A webgl react target that utilizes the Babylon.js library. Create 3D scenes and Games with React!
MIT License
44 stars 5 forks source link

Fixing Property Mismatch for Sphere scale primative #3

Closed cgrinker closed 7 years ago

cgrinker commented 7 years ago

The sphere shape component contains three properties to indicate scale:

diameterX,
diameterY,
diameterZ,

However the Mesh.CreateSphere method expects a single scaler for it's diamter parameter. Previously the Sphere component would pass a constant value 2 for diameter. This commit takes the magnitude of the three diameter properties, in order to prevent breaking anything using the component.

brochington commented 7 years ago

@cgrinker

Thanks for noticing this, and the pull request. Looking in Sphere.js:

const {
            segments,
            diameter = 2,
            children
        } = this.props;

the diameter prop isn't declared as a propType, but it is still handled. diameter = 2 is just a default value.

I need to update The Babylon version.

Also looking at the Babylon docs:

http://doc.babylonjs.com/classes/2.4/MeshBuilder#static-createsphere-name-options-scene-rarr-mesh-classes-2-4-mesh-

It looks like CreateSphere method now accepts an object of options, which include all of the diameter options. I think it should be possible to handle all of these options. Maybe for the time being we can use your magnitude technique if no diameter prop is specified. Either way diameter should be added to the proptypes.

Let me know if you would like to help out with any of this, as you are more than welcome too!