Refactor 2d_primitives.js

[JustZambetti]

trafficstars

Increasing Access

It would make the code shorter and more readable

Most appropriate sub-area of p5.js?

• [ ] Accessibility
• [ ] Color
• [X] Core/Environment/Rendering
• [ ] Data
• [ ] DOM
• [ ] Events
• [ ] Image
• [ ] IO
• [ ] Math
• [ ] Typography
• [ ] Utilities
• [ ] WebGL
• [ ] Build Process
• [ ] Unit Testing
• [ ] Internalization
• [ ] Friendly Errors
• [ ] Other (specify if possible)

Feature enhancement details

I think that 2d_primitives.js could be refactored a bit. These are some suggestions:

_normalizeArcAngles signature

Problems:

• function has too many arguments
• function has boolean argument, which requires, when reading the code, to see the function signature

From this:

p5.prototype._normalizeArcAngles = (
  start,
  stop,
  width,
  height,
  correctForScaling
)

To this:

p5.prototype._normalizeArcAngles = (
  size, //(width, height),
  drawRegion, //(startAngle, stopAngle)
  angleBehaviourOnScale //(angleBehaviourOnScale.Stretch, angleBehaviourOnScale.KeepRelative)
)

_normalizeArcAngles angle constraint

Code:

  // Constrain both start and stop to [0,TWO_PI).
  start = start - constants.TWO_PI * Math.floor(start / constants.TWO_PI);
  stop = stop - constants.TWO_PI * Math.floor(stop / constants.TWO_PI);

Problems:

• contains many levels of abstractions,
• code should expain itself

Proposal:

  drawRegion = ConstrainDrawRegionToOneCircumference(drawRegion);

_normalizeArcAngles abs

Code:

separation = Math.min(
    Math.abs(start - stop),
    constants.TWO_PI - Math.abs(start - stop)
  );

Problem:

• contains many levels of abstractions
• makes the function too long

Proposal:

arcAngle = getArchAngle(drawRegion);

_normalizeArcAngles angle scaling

Code:

// Optionally adjust the angles to counter linear scaling.
 if (correctForScaling) {
   if (start <= constants.HALF_PI) {
     start = Math.atan(width / height * Math.tan(start));
   } else if (start > constants.HALF_PI && start <= 3 * constants.HALF_PI) {
     start = Math.atan(width / height * Math.tan(start)) + constants.PI;
   } else {
     start = Math.atan(width / height * Math.tan(start)) + constants.TWO_PI;
   }
   if (stop <= constants.HALF_PI) {
     stop = Math.atan(width / height * Math.tan(stop));
   } else if (stop > constants.HALF_PI && stop <= 3 * constants.HALF_PI) {
     stop = Math.atan(width / height * Math.tan(stop)) + constants.PI;
   } else {
     stop = Math.atan(width / height * Math.tan(stop)) + constants.TWO_PI;
   }
 }

Problems:

• contains repeated code,
• contains numeric literals,
• contains a redundant comment,
• contains many levels of abstractions,
• is too long

Proposal:

  if(angleBehaviourOnScale === angleBehaviourOnScale.KeepRelative)
    makeDrawRegionRelative(drawRegion);
   
makeDrawRegionRelative(region){
  region.start = makeAngleRelative(region.start);
  region.stop = makeAngleRelative(region.stop);
  return region;
}

//I don't understand the math behind completely so I'm not able to simplify it further, 
// this method is actually too long but it's still an improvement
//TODO: simplify this method
makeAngleRelative(angle){
  const THREE_PI = 3 * constants.HALF_PI;
  let offsetAngle = 0;
  
  if(angle < constants.PI)
    offsetAngle = 0;
  else if (angle <= THREE_PI)
    offsetAngle = constants.PI;
  else 
    offsetAngle = constants.TWO_PI;

  return Math.atan(width / height * Math.tan(angle)) + offsetAngle;
}

_normalizeArcAngles angle last check

Code:

// Ensure that start <= stop < start + TWO_PI.
if (start > stop) {
  stop += constants.TWO_PI;
}

Problem:

• function contains many levels of abstractions Proposal:

drawRegion = makeStopGreaterThanStart(drawRegion);

But I would leave the comment inside of the function because it's not obvious

arc function signature

Problem:

• function has too many arguments but it's a public API so it's better to just group them and call another function

Proposal:

p5.prototype.arc = function(x, y, w, h, start, stop, mode, detail) {
  p5._validateParameters('arc', arguments);
  position: {x:x,y:y}
  size: {width:w, height: h}
  drawRegion: {start: start, stop: stop}
  return arc(position, size, drawRegion, mode, detail);
}

arc function premature return

Code:

// if the current stroke and fill settings wouldn't result in something
// visible, exit immediately
if (!this._renderer._doStroke && !this._renderer._doFill) {
  return this;
}
if (start === stop) {
  return this;
}

Problem:

• Code should expain itself so the comment should not be necessary

Proposal:

if(isArcNotVisible(this, drawRegion))
  return this;

arc function size abs

Code:

// p5 supports negative width and heights for ellipses
w = Math.abs(w);
h = Math.abs(h);

Problem:

• a function should only contain one level of abstraction

Proposal:

size = flipNegativeDimensions(size);

circle inline documentation

Problem:

• I don't think it is necessary to explain what a circle is.

 /* Draws a circle to the canvas. A circle is a round shape. Every point on the
 * edge of a circle is the same distance from its center. By default, the first
 * two parameters set the location of the center of the circle. The third
 * parameter sets the shape's width and height (diameter).
 */

circle function

Code:

  p5.prototype.circle = function() {
    p5._validateParameters('circle', arguments);
    const args = Array.prototype.slice.call(arguments, 0, 2);
    args.push(arguments[2]);
    args.push(arguments[2]);
    return this._renderEllipse(...args);
  }

Problem:

• code could be simplified

Proposal:

  p5.prototype.circle = function(x, y, diameter) {
   p5._validateParameters('circle', arguments);   
   return this._renderEllipse(x, y, diameter, diameter, diameter);
  }

_renderEllipse signature

Code:

p5.prototype._renderEllipse = function(x, y, w, h, detailX){
}

Problem:

• too many arguments

Proposal:

p5.prototype._renderEllipse = function(position, size, detailX){
}

_renderEllipse code

Code:

// p5 supports negative width and heights for rects
  if (w < 0) {
    w = Math.abs(w);
  }

  if (typeof h === 'undefined') {
    // Duplicate 3rd argument if only 3 given.
    h = w;
  } else if (h < 0) {
    h = Math.abs(h);
  }

##Opinion:

• I don't think this function should be so elastic to be able to accept different counts of arguments. It's only used internally after all.

Proposal:

size = flipNegativeDimensions(size);

I noticed that in many functions x and y, width and height are separate arguments but I think it's preferable to use position and size instead.

I would like to be assigned to this issue.

[davepagurek]

Hi, thanks for your interest in this! Some general comments:

• I'm not sure I fully agree with the bundling of some steps of algorithms into sub functions. To me, there's a balance to strike between function length and linearity so that one doesn't have to jump around when reading code. It might be obvious in some cases what a function is doing, so there isn't a need to look into its insides, but in other cases, it's really hard to write a function name that described what it's doing better than the code itself + a comment explaining why the choice was made. For example, adding TWO_PI to an angle or taking the absolute value of a number are things I'd probably want to leave as is. I'm curious how other maintainers feel about this though!
• Agreed that functions with a long list of positional parameters is hard to read at the call site.
• About internal methods handling lots of variants of arguments, e.g. negative inputs: I suspect this is so that we don't have to handle those inputs from public functions in multiple places. Is e.g. _renderEllipse called directly from multiple user-facing functions? If so, maybe it makes sense for it to internally adapt its inputs however it needs, and then pass those new inputs into another internal function that only accepts a cleaner subset of inputs?
• When making many changes that should have no impact on the functionality or public interfaces, changes should likely be accompanied by unit tests that ensure that the behaviour hasn't changed. We have some, but we'd want to take a look to make sure the coverage is good enough that we can rely on it to catch accidental errors.

[TanmayDhobale]

Hi @davepagurek, I noticed some issues in the 2D Primitives test suite and made the following improvements:

1. In the p5.prototype.ellipse suite, addressed the missing param #2 and #3 issues.
2. In the p5.prototype.rect suite, fixed the case where missing param #5 was not throwing a validation error.
3. Updated the p5.prototype.square suite to handle missing param #3 appropriately.
4. Fixed a bug in the p5.prototype.line suite where incorrect parameters were not triggering validation errors.
5. Improved error messaging in the p5.prototype.triangle suite for better clarity.

Before proceeding, I wanted to check if you have any specific considerations or if it's okay for me to submit a pull request with these changes. Looking forward to your feedback.

Best regards, tanmay

[limzykenneth]

There may be a case for this to be looked at as part of the 2.0 RFC at #6678. If so we can turn this into a proposal for refactoring the 2D renderer in general.