Add a template for p5 visualizers #293
Conversation
Also, add a "workbench" mode that shows visualizers under development.
Grab Vue version bump.
The environment variable is a string, not an integer.
There was a problem hiding this comment.
Here's a round of feedback. I tried to find as much stuff as I could, but I will have to go through again once these are all addressed. The point is that templates are important in setting up coding expectations, style, etc. and can have an outsized influence on the ease of future changes.
This comment was marked as resolved.
This comment was marked as resolved.
|
@katestange you forgot to |
|
@Vectornaut given the amount of feedback here I am going to forge ahead with the dependabot updates, so you will have to merge again. Really sorry, but we are under the gun. :-/ |
Thanks! (duh) It works as advertised on my machine. The simple visualizer of just showing the terms is a nice choice. |
|
The keypresses all worked? |
Indeed, yes, all three. |
|
Changed to draft because I believe there is more to come, and it will have to be rebased once #297 lands anyway. @Vectornaut please remove the draft designation when you feel it is ready for further review. |
Revise the p5 visualizer template and Differences visualizer to match.
|
I've reorganized and revised the instructions for how to build a visualizer. My main goals were:
Changes that especially need feedbackSince our convention is to call Why does Differences implement The previous revision said that when constructing a |
|
I haven't had a chance to look at what you wrote yet, but here is my two cents on the three items you raised in particular: (A)
Since there was boilerplate setup, I imagined there might be boilerplate draw code (e.g, p5 itself maintains a frame count; maybe P5Visualizer would eventually have some sort of housekeeping it wanted to do on every draw().) If you feel that is super unlikely, then we could make draw() an empty abstract method that must be implemented (with no super.draw()) in order for a visualizer class to be instantiable. In this scheme, for example, a SquareVisualizer would have no draw, indicating that it is solely a baseclass that visualizers who want a square canvas only can build on. (B)
Yes well, in the prior design, visualizer had initialize() and setup(), and there was a similar confusion, but in practice the convention was things that did not need access to the sketch were done in initialize and things that did in setup. Now inhabit implicitly calls setup, so the distinction has become even blurrier. But the fact is, setup is a p5 method and inhabit is an underlying visualizer method, so it might make organizational sense to use the same division as before. Certainly it is still the case that anything that needs the sketch must be in setup. So the question is, where do we want to promote rank-and-file visualizers doing other startup type things that don't need the sketch? Inhabit() or setup()? I don't see much to choose between them... but yes, we don't want this to be a confusing point. There's one other thing I am worried about: with a click-free, parameters-modified-on-the-fly UI like we want, there will be a distinction between restarting a visualizer and departing/reinhabiting it, that doesn't exist now. For example, imagine changing the stroke color of a Turtle visualizer in progress. Does that just make the rest of the path (from where the turtle currently was) the new color, or does it erase everything, and go back to the start location and start drawing from the first term again with the new color? Conceptually I would like there to be the option of visualizers that can be altered midstream. But then that raises the question of who decides? Is it purely internal to the visualizer, or can the Vue app tell a visualizer to "take it from the top"? If so, there needs to be a visualizer method that corresponds to this directive, and then there will be a very practical distinction between one-time prep and stuff that needs to be done every time you take it from the top. When I started with numberscope, I presumed that setup() was the "take it from the top" method, but no, p5 is very clear that setup is to be called once in the lifetime of a sketch. So that blurs the distinction between inhabit and setup, and it means we will at some point soon likely need a third method for "take it from the top". I guess I am OK with, for p5-based visualizers, saying that any startup stuff that doesn't have to do with the DOM should be in setup(), and modifying all existing visualizers that way. If that seems like the best plan, you could document it that way in this PR and change Differences to conform (so it would have no inhabit method, I think think), and then in a second PR we could conform all the others. I don't think this PR should be altering visualizers across the board. Let me know your thinking on (B) in light of all the above background and discussion. (C)
Yeah this is mysterious to me, too. It has just been left alone since before I came on the scene, since I never really understood what's going on here with these comments. But if we are doing a cleanup that extends to this, then I would love as part of this PR to remove the redundancy between name in the visualizer class and in the exportModule. It's not DRY. Thoughts on how to do so? |
|
OK, Aaron and I had an in-depth discussion today and we reached the following conclusions on (A) and (B) above: (A) Since there already is a boilerplate wrapper around the draw method, there will never be a need for a common P5Visualizer.draw(). Therefore, we will make it abstract, and hence required to be implemented in all instantiable derived classes. (B) Since setup() is the designated initialization place for p5 sketches, we will adopt the convention that any startup code that does not need to deal with the DOM directly or the call to I am about to submit a PR making these changes. As a bonus, I will deal with (C) as well: make visualizer files DRY in only having their name in one place in the code. |
Three related interface changes for the Visualizer hierarchy, as discussed in #293. These are all designed to make some of the details of defining a Visualizer less confusing and more concise. (A) In P5Visualizer, draw() should be abstract and hence required on runnable derived Visualizers, since any (additional) boilerplate needed by all P5Visualizers would go in the function that wraps this method in the default inhabit() method. Thus implementation draw() functions no longer need to call super.draw(). (B) Since setup() is the default p5 initialization method and can only be called once per sketch anyway, clarify that generally initialization should be carried out in setup() rather than inhabit(), in derived Visualizers. (C) Since `name` is used as an instance property in SequenceInterface indicating the details of the particular interface, re-designate that information as the return value of a `visualization()` method of an entity implementing the VisualizationInterface, and suggest that it should only depend on the class of the entity, not the instance. Implement this in the P5Visualizer hierarchy with the visualizationName static property, and modify the VisualizerExportModule constructor to look for it there, so that the visualizationName will not have to be stated twice in every module implementing a Visualizer. At such time as there are other base classes for concrete Visualizers, these conventions should probably be moved one level up to a common base class of all visualizers; there is nothing specifically related to p5 about this naming scheme.
Three related interface changes for the Visualizer hierarchy, as discussed in #293. These are all designed to make some of the details of defining a Visualizer less confusing and more concise. (A) In P5Visualizer, draw() should be abstract and hence required on runnable derived Visualizers, since any (additional) boilerplate needed by all P5Visualizers would go in the function that wraps this method in the default inhabit() method. Thus implementation draw() functions no longer need to call super.draw(). (B) Since setup() is the default p5 initialization method and can only be called once per sketch anyway, clarify that generally initialization should be carried out in setup() rather than inhabit(), in derived Visualizers. (C) Since `name` is used as an instance property in SequenceInterface indicating the details of the particular interface, re-designate that information as the return value of a `visualization()` method of an entity implementing the VisualizationInterface, and suggest that it should only depend on the class of the entity, not the instance. Implement this in the P5Visualizer hierarchy with the visualizationName static property, and modify the VisualizerExportModule constructor to look for it there, so that the visualizationName will not have to be stated twice in every module implementing a Visualizer. At such time as there are other base classes for concrete Visualizers, these conventions should probably be moved one level up to a common base class of all visualizers; there is nothing specifically related to p5 about this naming scheme.
|
OK, all of the streamlining we discussed on the VisualizerInterface/P5Visualizer is now submitted in PR #310. I think the best way forward is to merge that into main (so feel free to go ahead and review it and once you are satisfied, merge it), and then for you to rebuild this on top of the changes, tweaking your documentation (and template visualizer) to reflect the changes. But if you have another plan that you think would work better, just let us know. |
|
Another question: Should the MouseTester visualizer from the discussion of #277 go into the workbench created in this PR? If so, in this PR or a subsequent one? |
I think that's a good idea, but priority isn't high, so I don't have a strong opinion when. And another issue to request that the MouseTester be expanded to show each type of interaction even, if anyone were to find that useful someday. |
Maybe EventTester would be a better name, or something like that. |
Three related interface changes for the Visualizer hierarchy, as discussed in #293. These are all designed to make some of the details of defining a Visualizer less confusing and more concise. (A) In P5Visualizer, draw() should be abstract and hence required on runnable derived Visualizers, since any (additional) boilerplate needed by all P5Visualizers would go in the function that wraps this method in the default inhabit() method. Thus implementation draw() functions no longer need to call super.draw(). (B) Since setup() is the default p5 initialization method and can only be called once per sketch anyway, clarify that generally initialization should be carried out in setup() rather than inhabit(), in derived Visualizers. (C) Since `name` is used as an instance property in SequenceInterface indicating the details of the particular interface, re-designate that information as the return value of a `visualization()` method of an entity implementing the VisualizationInterface, and suggest that it should only depend on the class of the entity, not the instance. Implement this in the P5Visualizer hierarchy with the visualizationName static property, and modify the VisualizerExportModule constructor to look for it there, so that the visualizationName will not have to be stated twice in every module implementing a Visualizer. At such time as there are other base classes for concrete Visualizers, these conventions should probably be moved one level up to a common base class of all visualizers; there is nothing specifically related to p5 about this naming scheme.
|
Great then, for expediency, once this is merged let's just file an issue to add a visualizer into the workbench that exercises every type of interaction. |
This comment was marked as resolved.
This comment was marked as resolved.
Re-order and re-name "Get started" sections to encourage people to document as they write. Turn the former "Abstract visualizers" section into a visualizer interface reference on a separate doc page.
Also, update the p5 template image, and add Glen's big guidance comment at the top of the template.
|
I think I've addressed all the biggest feedback, leaving only stuff that can reasonably be dealt with in future PRs. I should be available again around noon pacific. Feel free to make and push small changes; I can review them when I'm online again if needed. Thanks again for all the careful checking and workshopping, @gwhitney and @katestange! |
(A) In refreshing parameters, there may have been some confusion between "the user" and "you", depending on the reader's perspective. Amplified that difference. (B) Made the convention more consistent in the detailed look at p5 Visualizer that headings for data properties are nouns, methods start with verbs, and other structural/procedural aspects start with "How to".
|
OK, to the best of my knowledge all of the items above this comment are resolved, except for if @katestange wants to take a look at the link to the event-handling methods in the making a visualizer doc (and perhaps express a preference for an explicit list in our docs), see #293 (review). So I would be ready to approve for merge except for one new real concern that I think we should at least discuss before merging. The example visualizer, if you run it, just puts up a big number with a blue rectangle below it, and then as far as the person looking at it can tell, stops. There is no clue to the further behavior possible -- that person would just have to guess that maybe some keys did something, and start hitting some more or less at random. That seems at odds to me with some of the "no barriers" principles we just elucidated in our Delft meeting, and so I am not sure we want to model that kind of opaqueness of visualizer behavior in our template. That suggests that What say you, my colleagues? And @Vectornaut, congratulations on a major PR -- you should be proud of how this has come out! |
It's perfect.
For (A) let's add an issue and mark it UI and mention it to the Delft team; (B) yes, something visual, as unobtrusive as possible, I agree. Here's an idea for a simple visual clue, although Glen's second suggestion is perhaps simpler: https://images.app.goo.gl/BkEBS8VCPL6PsxjU6 |
|
Agreed on both counts. In the interest of expediency (it's well time to move this along) I just checked in a small commit that puts the text below the bar. Aaron, feel free to change it as you see best if you feel some other hint would fit in the minimalist graphic design better. At this point, when you're happy we're happy and will merge this! In fact, I will give my approval now. |
Links didn't change with section headings.
|
These changes look reasonable! I've tweaked the "never really finished" comment, fixed some links that broke when headings changed, polished the template visualizer's controls hint, and shrank the template visualizer sample image. Happy to follow up on other issues in future PRs. |
By submitting this PR, I am indicating to the Numberscope maintainers that I have read and understood the contributing guidelines and that this PR follows those guidelines to the best of my knowledge. I have also read the pull request checklist and followed the instructions therein.
This update adds a new visualizer directory,
src/workbench, whose contents don't normally show up in the visualizer list. To use the visualizers in this directory, you serve Frontscope in "workbench mode" by callingnpm run dev:workbench.If this pull request looks generally okay, I'll go on to document the workbench feature.
Right now, there's only one workbench visualizer: "p5 Visualizer Template", also added in this update.