[SOLVED] createCylinder radius opt parameter inconsistent in code and documentation

Suggestions & Feedback
#1

https://developer.playcanvas.com/en/api/pc.html#createCylinder
states

opts.radius Number The radius of the tube forming the body of the cylinder (defaults to 0.5).

but in procedural.js the code uses baseRadius not radius.

pc.createCylinder = function (device, opts) {
...
var baseRadius = opts && opts.baseRadius !== undefined ? opts.baseRadius : 0.5;
#2

Feel free to make PR to the engine: https://github.com/playcanvas/engine

#3

Yep, should be radius instead of baseRadius in the code.

#4

Won’t changing opts.baseRadius to opts.radius be a breaking change? Doesn’t this need to be done defensively, deprecating opts.baseRadius?

e.g.

if(opts.baseRadius !== undefined) {
  console.log("'baseRadius' deprecated! Please use radius.")
  if(opts.radius === undefined)
    opts.radius = opts.baseRadius;
}
#5

Logging should be behind DEBUG define, like:

// #ifdef DEBUG
if (opts.hasOwnProperty('baseRadius') && ! opts.hasOwnProperty('radius'))
    console.warn('DEPRECATED: "baseRadius" in arguments, use "radius" instead');
// #endif

And for simplicity of logic bellow:
It makes no sense to have radius of 0, so not checking for property, but value.

opts.radius = opts.radius || opts.baseRadius;

And yes, no breaking is allowed, we have to make sure old way still works.

#6

This won’t issue a warning if both radius and baseRadius are supplied. Not a problem?

#7

Not a problem, same as if you provide any other random property, there won’t be warning for it.

#8

Done.

Took a while, still getting used to GitHub/Desktop.

1 Like
#9

Thanks for your contribution! U rock! :guitar:

#10

Aw shucks! ‘Tweren’t nuthin’.