numberscope / frontscope

Numberscope's front end and user interface: responsible for specifying sequences and defining and displaying visualizers
MIT License
7 stars 14 forks source link

refactor: Separate p5-specific code into a P5Visualizer base class #277

Closed gwhitney closed 2 months ago

gwhitney commented 2 months ago

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 the views/components have been changed so that said div takes up all of the 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.

gwhitney commented 2 months ago

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.

import {VisualizerExportModule} from '@/visualizers/VisualizerInterface'
import {P5Visualizer} from './P5Visualizer'
import type p5 from 'p5'
/** md
# Mouse Tester
**/

class MouseTesterVisualizer extends P5Visualizer {
    name = 'Mouse Tester'
    colorValuePressed = 0
    colorValueReleased = 0
    colorValueKeyPressed = 0
    colorValueClicked = 0
    params = {}

    draw(sketch: p5) {
        super.draw(sketch)
        const downSet = 80
        const offset = 10
        sketch.textSize(20)

        sketch.fill('blue')
        sketch.text('press mouse to change colour', downSet, downSet)
        sketch.fill(this.colorValuePressed)
        sketch.rect(downSet, downSet + offset, 50, 50)

        sketch.fill('blue')
        sketch.text('release mouse to change colour', downSet, 2 * downSet)
        sketch.fill(this.colorValueReleased)
        sketch.rect(downSet, 2 * downSet + offset, 50, 50)

        sketch.fill('blue')
        sketch.text('press key to change colour', downSet, 3 * downSet)
        sketch.fill(this.colorValueKeyPressed)
        sketch.rect(downSet, 3 * downSet + offset, 50, 50)

        sketch.fill('blue')
        sketch.text('click mouse to change colour', downSet, 4 * downSet)
        sketch.fill(this.colorValueClicked)
        sketch.rect(downSet, 4 * downSet + offset, 50, 50)
    }

    mouseReleased() {
        if (this.colorValueReleased === 0) {
            this.colorValueReleased = 255
        } else {
            this.colorValueReleased = 0
        }
    }
    mousePressed() {
        if (this.colorValuePressed === 0) {
            this.colorValuePressed = 255
        } else {
            this.colorValuePressed = 0
        }
    }

    keyPressed() {
        if (this.colorValueKeyPressed === 0) {
            this.colorValueKeyPressed = 255
        } else {
            this.colorValueKeyPressed = 0
        }
    }

    mouseClicked() {
        if (this.colorValueClicked === 0) {
            this.colorValueClicked = 255
        } else {
            this.colorValueClicked = 0
        }
    }
}

export const exportModule = new VisualizerExportModule(
    'Mouse Tester',
    MouseTesterVisualizer,
    'Check if p5 mouse control works.'
)
Vectornaut commented 2 months ago

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.

gwhitney commented 2 months ago

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.

gwhitney commented 2 months ago

Thanks for the review!

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?

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: (A) I had thought that it was a design direction that had been discussed and that we wanted to go, for the visualizer canvas to (i) not extend "off-screen" and hence possible require scrolling to see the whole picture, but (ii) otherwise make as much space as possible available for visualization? And that then we would have a sub-base-class of P5Visualizer maybe called SquareVisualizer that pared the canvas down to the largest square that will fit in the available space, and center it therein? (Then visualizers that only want to be on a square could just derive from SquareVisualizer.) Is that re-shaping of the canvas indeed the way we want to go, and if so, do we want to do that as part of this PR, or do you want me to adjust it back to fixed-size square canvases for this PR and we will address sizing issues during the Delft Team Project? (B) If we go ahead with the re-shaped and dynamic-sized canvas for this PR, do you want me to patch up all of the visualizers that are having trouble with re-sizing as part of this PR, or merge it and file issues for those visualizers that need tweaking as a result?

(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;

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!

(3) the advertised functionality works!

Yay!

(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)

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 visualizerTemplates (so that it won't show up on screen in production), so that you can just copy ExampleP5Visualizer.ts to the visualizer director under a different name and modify it to make your own visualizer. It would be commented with all of the places you need to make changes, and use each of the features -- parameters, setup, draw, event function, etc., -- once so you could see how each was done. But you raise a good point -- I did not check the documentation for references to VisualizerDefault and amend them if so, possibly leaving the documentation inconsistent. (Sorry, this was a bit of a rush job -- getting TypeScript to be OK with all of the possible but unimplemented p5 methods took hours and some bit of arcane code in the end, :-/ ) I will do that in a new commit asap.

(5) I found a bug in github's reviewing interface :)

Interesting. I assume no action item for us on that?

katestange commented 2 months ago

So this opens a couple of sub-concerns: (A) I had thought that it was a design direction that had been discussed and that we wanted to go, for the visualizer canvas to (i) not extend "off-screen" and hence possible require scrolling to see the whole picture, but (ii) otherwise make as much space as possible available for visualization? And that then we would have a sub-base-class of P5Visualizer maybe called SquareVisualizer that pared the canvas down to the largest square that will fit in the available space, and center it therein? (Then visualizers that only want to be on a square could just derive from SquareVisualizer.) Is that re-shaping of the canvas indeed the way we want to go,

This matches my memory, yes.

and if so, do we want to do that as part of this PR, or do you want me to adjust it back to fixed-size square canvases for this PR and we will address sizing issues during the Delft Team Project?

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.

(B) If we go ahead with the re-shaped and dynamic-sized canvas for this PR, do you want me to patch up all of the visualizers that are having trouble with re-sizing as part of this PR, or merge it and file issues for those visualizers that need tweaking as a result?

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.

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!

Will do.

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 visualizerTemplates (so that it won't show up on screen in production), so that you can just copy ExampleP5Visualizer.ts to the visualizer director under a different name and modify it to make your own visualizer. It would be commented with all of the places you need to make changes, and use each of the features -- parameters, setup, draw, event function, etc., -- once so you could see how each was done. But you raise a good point -- I did not check the documentation for references to VisualizerDefault and amend them if so, possibly leaving the documentation inconsistent. (Sorry, this was a bit of a rush job -- getting TypeScript to be OK with all of the possible but unimplemented p5 methods took hours and some bit of arcane code in the end, :-/ ) I will do that in a new commit asap.

Sounds good.

(5) I found a bug in github's reviewing interface :) Interesting. I assume no action item for us on that?

Nope.

gwhitney commented 2 months ago

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!

gwhitney commented 2 months ago

I did not check the documentation for references to VisualizerDefault and amend them if so, possibly leaving the documentation inconsistent. (Sorry, this was a bit of a rush job -- getting TypeScript to be OK with all of the possible but unimplemented p5 methods took hours and some bit of arcane code in the end, :-/ ) I will do that in a new commit asap.

Sounds good.

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 this.sketch" proposal, I am going to wait until I hear some feedback on that before reviewing the docs. If that's the way we are going to go, then I will do that change and the doc review in the same new commit.

katestange commented 2 months ago

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!

Sounds perfect, thanks! I noticed Histogram axes labels, as I pointed out, but I will have to investigate more what's happening with others.

gwhitney commented 2 months ago

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.

gwhitney commented 2 months ago

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...)

Vectornaut commented 2 months ago

I'm fine with merging, and it would be helpful for me in practicing with the visualizer system. Done!