-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Separate p5-specific code into a P5Visualizer base class #277
Conversation
Correspondingly, remove all p5-specific code from the Vue views and components. This means the Visualizer code is handling all the canvas operations, so p5-based visualizers have access to the canvas. In addition, any of the standard p5 methods that are present as methods of the visualizer are installed on the sketch, so they will be called by the sketch when the associated events occur. Finally, this commit adds methods to dispose of the visualizer, to avoid multiple dangling sketch instances, which could otherwise cause events to be multiply processed. Resolves numberscope#120. Resolves numberscope#272. Possibly it also handles numberscope#244; it should be checked after this is merged.
You can verify that the events @katestange was most interested in all work by copying the below file as MouseTester.ts in the visualizers directory, and trying out the events.
|
For what it's worth, I can build and launch this version using Node 21.7.3. The mouse tester visualizer behaves as expected, and the other visualizers I tried looked reasonable at a glance, although I didn't make any serious attempt to validate their output. @katestange, I'd imagine you're in a better position to review the code, because you're more familiar with Frontscope. I could try to look at the code Saturday evening if it would be helpful, though. |
A thing that would be useful if you happen to have time before the merge is simply play with some of the other visualizers to make sure they seem unaffected by the change. Hopefully Kate will be able to do a review, yes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, mainly my comments are just requests for explanations, just so I can follow what's going on and be prepared to write visualizers in future. I think most of the comments and questions I have either require just an explanation, or creating an issue to deal with things that have arisen separately.
Some things I'm observing:
(1) the new canvas changes size with browser window and a variety of visualizers are having a hard time with this (including histogram, where the x axis labels float around, that's certainly my bad) -- I'm assuming we should file separate issues for these instead of fixing them with this PR?
(2) I've been using my own backscope for some time since we were working on it but I tried it with the production backscope and it wouldn't connect due to CORS errors -- again, I'm assuming this is unrelated and I will file/investigate separately;
(3) the advertised functionality works!
(4) VisualizerDefault.ts was deleted; I suppose part of the future roadmap is to make a replacement example visualizer file? (But not part of this PR)
(5) I found a bug in github's reviewing interface :)
src/visualizers/Chaos.ts
Outdated
@@ -376,8 +384,8 @@ class Chaos extends VisualizerDefault { | |||
this.sketch.strokeWeight(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the modifications to the visualizers a bit more? I've noticed many changes are referring to sketch
not this.sketch
, but not everywhere (e.g. here). It looks like here there's some issue that sketch
isn't working quite yet so we need to fall back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is a bit of a long story, sorry. (Too bad we don't have time left for a meeting on this.) We were never exercising the p5 event handling, as you know, because it didn't work because of the way Numberscope had structured the interaction between the Vue framework and the visualizer code. So when I turned that on, events were indeed being sensed, but they were all being executed multiple times, and the more I played with the system, the more and more times each event was being processed. Finally I figured out that every time you hit a "Draw" button on a BundleCard and then every time you hit the "<Back" button on the main drawing page, a new p5 sketch was being created, which isn't so bad, but all of the old p5 sketches, even though they were no longer in the active DOM, were still running and plotting themselves and doing everything including processing events, but just not being displayed because they were no longer in the DOM.
So with the current design of the Vue framework, I had no choice but to add methods to dispose of the old sketch instances when what's active in the DOM changes. But that in turn means that the type of the sketch
member of a P5Visualizer had to change from p5
to p5 | undefined
, because it truly may be the case that for some stretch of a visualizer's lifetime, after it has been created and maybe after it at one time had a valid sketch, it might not have any sketch (if the framework has temporarily shelved it with dispose()
but might later revive it by having it inhabit()
a new DOM element, when it rearranges the screen).
Therefore, typescript wants all the code to deal with the possibility that this.sketch
might be undefined. And it is basically right, except for the fact that for most of the member functions, at the point they are called, there really will be a valid sketch object in the this.sketch
property. But to keep the TypeScript compiler happy (and, frankly, to be really sure the code won't under any circumstances barf), all the functions need to be sure to work when this.sketch comes in undefined.
The two sort of special cases here are setup()
and draw()
:
(A) When setup
is entered, this.sketch() is always undefined -- it is setup's responsibility to create the sketch. But that happens in P5Visualizer.setup(), and all derived p5 visualizers are required to call super.setup() so that the sketch will be initialized. So after that call to super.setup(), this.sketch is guaranteed to be valid. I wasted an hour or so trying to get TypeScript to realize that once super.setup() returns successfully, this.sketch is automatically valid, but I failed to find a way to do so. That's why you see all derived setup() functions (redundantly) discarding the possibility that this.sketch might be undefined, just after calling super.setup(). If anyone has any ideas how to smooth this, I am all ears -- I did try.
(B) When draw
is entered, this.sketch is absolutely guaranteed to be valid -- with the design of p5 and the P5Visualizer class, there is no way draw()
can be called without a valid sketch, because it is the sketch itself that is calling draw! But TypeScript has no way of knowing this. Moreover, draw is the place where 90% of the uses of this.sketch occur anyway. So I compromised and changed the structure of P5Visualizer so that it would pass in that certainly valid sketch as the sketch
argument to draw. It is guaranteed to be equal to this.sketch, and it is guaranteed to be a valid p5 sketch so you don't have to runtime typecheck the sketch argument to draw to make TypeScript happy, like you would the this.sketch property. And it also seemed like a bit of a win because since the big bulk of sketch references occur in draw, so being able to refer to the sketch just as the sketch
argument rather than the this.sketch
property also allowed removing dozens of occurrences of this.
from the code, making it more concise. So the upshot -- which I grant may be a bit confusing -- is that the sketch
argument to draw()
is guaranteed to be this.sketch
, just pre-type-certified as valid for the TypeScript compiler.
Again, if you want to revisit this design/handling of the sketch object in the P5Visualizer hierarchy, I am super happy to if anyone can come up with a smoother system. I just couldn't see any alternative to allowing the possibility in the code that this.sketch might be undefined, because the framework really does need the ability to throw away a sketch after it has done some amount of drawing. And so the visualizer has no way of knowing for sure at any point in its lifetime that this.sketch is currently valid, as far as I could see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Holy Cow! Ok. I'm actually glad this is in writing, not just in a zoom meeting. I suggest @Vectornaut link to this comment or discussion as a code comment in the example visualizer code he will be adding, so that when someone else wonders, they can dive into it if they care, and so they don't try to circumvent it or get confused. If there's anywhere appropriate to add a link or explanation in a comment inside the code in this PR, that would be great. Otherwise, I think it is what it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah actually in thinking about this situation more in the context of your Grid.ts comment a few conversations below, I did come up with a couple of other design tweaks that seems like it could streamline this considerably. So before responding here, do take a look at the options there and let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was another example of conversation overlap. Will be interested to hear what you think about the new proposal.
src/visualizers/Grid.ts
Outdated
@@ -633,6 +633,10 @@ earlier ones that use the _same_ style.) | |||
} | |||
|
|||
setup(): void { | |||
super.setup() | |||
if (!this.sketch) { | |||
throw 'Attempt to show Grid before injecting into element' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the consistent changes happening here, I wonder if there should be any accompanying documentation. I know we don't have properly documented info on how to build visualizers yet, but this construction here for example may need some explanation somewhere? Not sure if that's in the scope of this PR -- probably more appropriate to create a nicely commented/documented example visualizer template. So I'll leave this comment here for now and we can maybe open an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to say something brief in the doc review/commit I am about to do, and then hopefully we can get more on this in a ExampleP5Visualizer.ts that Aaron may have time to do tomorrow. (Or even better if one of you figure out a cleverer way to do this so that setup can be written more smoothly in the Visualizers derived from P5Visualizer...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just something to go to @Vectornaut for the example visualizer, probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, we are overlapping in my ability to respond (and consider the options here)... see my response to the next code comment below for some other options here that will likely make the derived Visualizers code look/feel noticeably smoother, and let me know what you think.
src/visualizers/Grid.ts
Outdated
@@ -822,7 +827,7 @@ earlier ones that use the _same_ style.) | |||
showNumber() { | |||
const currentNumberAsString = this.currentNumber.toString() | |||
|
|||
if (this.showNumbers) { | |||
if (this.sketch && this.showNumbers) { | |||
this.sketch.fill(this.numberColor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, this change looks kind of like a fall back check whether this.sketch
is a thing? Can you explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, hopefully the above explanation made it clearer why an arbitrary member function of a Visualizer derived from P5Visualizer must now cope with the possibility that this.sketch is undefined. Another possibility for the Grid visualizer adjustment to the change would be as follows: ultimately, showNumber is called from draw (either directly or indirectly). And draw() gets a valid sketch argument. So we could instead change showNumber to take a sketch argument and just use the sketch argument, instead of dealing with this.sketch at all. Then it can assume the sketch argument is valid. We would just have to make sure the sketch argument is passed down through the call chain from draw() to showNumber().
Indeed, saying this suggests to me another global approach to the whole "sketch may not be valid" thing, which I could adjust this PR to use if you like: the actual sketch object could be kept in a different property private to P5Visualizer, and it could instead implement a read-only this.sketch property via a getter that would always return a valid sketch object, or throw if it is not able to. But since all of the calls actually happen with valid sketches, it would never actually throw. That would mean putting back in all of the this.
before the sketch references in draw, and removing all of the new typechecking on this.sketch, so there would be many fewer individual code changes in this PR. On the other hand, under the hood there would be lots more times when the resulting javascript is checking whether this.sketch is undefined.
A middle path would be to do this private sketch/getter thing, but in the big draw() functions that use the sketch lots of times and where I have already changed this.sketch
to sketch
in numerous occurrences, to just put a line const sketch = this.sketch
at the top. Then you get the hidden typecheck only once, and you get the conciseness of writing sketch
rather than this.sketch
, while allowing all of the other functions that only use the sketch once or twice to just say this.sketch
and not to have to do any explicit typechecking.
Let me know what path you want to go on this. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, what you suggest last (middle path) seems reasonable, but I don't have much expertise here. Is the hidden typechecking a significant overhead really? If so, then I'd like to avoid it (since it is running on everything we do inside the draw() loop). But I have no idea if it's an efficiency concern. Ultimately, I trust you to make a decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. I am not really a javascript performance expert. But given the following considerations:
(A) We want visualizer writing to be as smooth as possible, and just being able to refer to this.sketch
seemed to be pretty comfortable before;
(B) Any one check for undefined before actually using the sketch is presumably of microscopic cost, the only issue is if they really pile up into a very large number of redundant such checks;
(C) In many circumstances, one such check is basically unavoidable because the framework may well have decided to dispose of a previously-existing sketch, and
(D) In any one function, you can get down to just one such check by writing const sketch = this.sketch
and then using the local sketch variable,
I am going to go ahead with this middle path. I will try to get the updated commit in ASAP and let everyone know when it is there. In the meantime I will mark this as draft.
Thanks for the review!
OK, yes, this is a behavior change resulting from this PR that I should have documented in the commit message. I have now edited it to mention this change. And mea culpa -- I did not run through all of the visualizers to see how they coped with the possibility that the canvas could change size on the fly. So this opens a couple of sub-concerns:
Sounds like we may have created a regression in the CORS headers in one of the recent changes to backscope. I don't think we have a CORS test :( -- so yes, absolutely file a backscope issue and mention that we need an active test for it!
Yay!
Well, actually, VisualizerDefault.ts was renamed to P5Visualizer.ts but it became so different that git doesn't track it as the same file. And VisualizerDefault was not a good template for making a visualizer, anyway, really. @Vectornaut is hoping to have time tomorrow for a PR checking in an ExampleP5Visualizer.ts in a new subdirectory
Interesting. I assume no action item for us on that? |
This matches my memory, yes.
I think you should decide based on the work-life balance this weekend. :) We are down to the wire for the Delft startup and probably we should have something stable, as the first priority. I'm also ok with implementing resizing but saving "square" for later, if that's the easiest path. I leave it to you to decide.
As above, depends on the time you have. If you file separate issues I can help with those individually and they don't need to be done in time for Delft.
Will do.
Sounds good.
Nope. |
OK, on (A) and (B) in the non-code-review discussion here, since resizing is now working and that is the overall direction you want to go, in the interest of expediency I would like to move this PR forward with the behavior change, and file issues for any visualizers that would (I) benefit from the existence of a SquareVisualizer that they could derive from instead of directly from P5Visualizer, and/or (II) need some amount of enhancement/updating to deal with a canvas that may change sizes on the fly in any case. If there are already ones you know of that fall into (I) and/or (II), please list them here so that we can file issues for them immediately upon this being merged, thanks! |
On this one, since the new docs will depend on whether we smooth the sketch access for derived classes with the "private sketch/checking getter in |
Sounds perfect, thanks! I noticed Histogram axes labels, as I pointed out, but I will have to investigate more what's happening with others. |
Adding to this list, Grid implicitly assumes the canvas is square, so some of the grid is cut off when it is running on a non-square canvas. Also, I got one "factorization is undefined" popup error, but I couldn't reproduce it... And it appears ModFill assumes square as well. |
Also simplifies Differences.ts a bit, since it is used as the example visualizer in the "making-a-visualizer" doc page, and updates said doc page to correspond to the changes in deriving from P5Visualizer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I'm seeing the desired behaviour, and our longer conversations from earlier seem to be resolved. I'm ok with merging. Should I go ahead or wait for @Vectornaut ?
Well, the things that are waiting on this are your getting the Glyph visualizer in, and Aaron doing a visualizer that is more explicitly a template for creating a new visualizer than Differences, if he feels it is still necessary after reading the latest version of making-a-visualizer.md. So I would say that if you wanted to work on Glyph tonight and it would be simpler for you to do so with this merged, it's fine for you to merge it now; if not, it's fine to wait a bit in case Aaron wants to take a look in the morning, especially as it's a big-ish PR. In general I'm a strong believer in at least two people being involved in a PR (the author and a reviewer) but don't have a strong feeling in general that PRs need three folks, for a project at this level of activity. (I am also working on WebKit, the underlying library of Safari, and for that it's absolutely the case that at least three people are involved...) |
I'm fine with merging, and it would be helpful for me in practicing with the visualizer system. Done! |
Correspondingly, removes all p5-specific code from the Vue views and
components. This means the Visualizer code is handling all the canvas
operations, so p5-based visualizers have access to the canvas.
In addition, any of the standard p5 methods that are present as methods
of the visualizer are installed on the sketch, so they will be called
by the sketch when the associated events occur. Moreover, as the
P5Visualizer is now creating the canvas, it chooses the size to fill the DOM
div
element that has been passed to it as its location, and the CSS of theviews/components have been changed so that said
div
takes up all ofthe screen space not allocated to other parts of the display. This means
that the size of the p5 canvas is subject to change, which some visualizers
may not yet fully adjust to properly. Finally, this commit adds methods
to dispose of the visualizer, to avoid multiple dangling sketch instances,
which could otherwise cause events to be multiply processed.
Resolves #120.
Resolves #272.
Possibly it also handles #244; it should be checked after this is merged.
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.