Skip to content

Conversation

lorenzo-lipparini
Copy link

This pull request adds a lineDash() function to the p5 prototype which provides an easy way to draw dashed lines.
The implementation is based on the native CanvasRenderingContext2D.getLineDash() / CanvasRenderingContext2D.setLineDash() methods.

The changes include:

  • lineDash() method definition in p5.Renderer2D class
  • lineDash() method definition in p5 class
  • A description of the function based on this MDN article
  • A few examples for the p5 documentation page

As this is my first pull request to the p5 project, I had a few problems with the parameter validation system which I didn't manage to get to work, so I'm looking for some help on that. Furthermore, I have no idea how to write tests for the new functionality, so I'll need help on that as well.

There are a few things about the API which I'm not sure about that I'd leave to someone more experienced than me to decide:

  • The lineDash() function accepts an array as its only argument for now, while we might consider to use rest parameters instead
  • The lineDash() function always returns the current line dash settings, and is therefore not chainable
  • The lineDash() function can be called with no parameters (it just returns the current settings)
  • A noLineDash() function equivalent to calling lineDash([]) might be a good addition, since it would probably make the API more clear
  • There is no support for the function in WEBGL mode, and no "friendly debug log" is provided if it gets called in that context (which is the case also for other p5 functions such as strokeCap() and strokeJoin())

Looking forward to receiving your suggestions.

@limzykenneth
Copy link
Member

Usually we suggest opening an issue with the feature request first to discuss the implementation of the feature and whether it is something we want to include in the library, just so that if the implementation needs to be changed or the feature is rejected, you won't have spent too much of your time working on it.

I have some comments about this implementation but can you close this PR for the moment and open an issue for the discussion? Thanks!

@lorenzo-lipparini lorenzo-lipparini mentioned this pull request Jun 13, 2018
2 tasks
@lorenzo-lipparini
Copy link
Author

Ok, the issue (#3016) is now open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants