Improvements to navigation-2d

[zsrail]

This PR addresses issue #366 and implements the changes I suggested there. In particular, it adds a macro, with-navigation-2d, and an option, :wrap-draw, which together can be used to make navigation-2d wrap part of the user's draw function without wrapping all of it. This is useful for sketches that have UI elements or other things that shouldn't be affected by navigation.

This PR also makes navigation-2d wrap all the mouse handlers in order to pass them the "world-coordinates" of the mouse in addition to the usual screen-coordinates. That way, the user's handlers can compare the mouse's world-coordinates to the coordinates they're drawing stuff at in their draw function. Otherwise, the handlers would have no way of telling if the mouse was close to objects affected by navigation-2d without looking at navigation-2d's state. A function, mouse-world-coords, is exposed so that the user can get the world-coordinates of the mouse in other contexts, like in the draw function.

Finally, two other functions, world->screen-coords and screen->world-coords are exposed in case the user needs to convert between the two coordinate systems for any other reason.

Let me know if any adjustments need to be made.

[zsrail]

I just fixed a couple formatting issues that were caught by running the tests. I also realize that I need to write snippets for the new public functions, so I'm converting this PR to a draft until I do that.

[zsrail]

I've added the snippets. In the process I also added :width and :height options to navigation-2d so that I could apply navigation to an image that didn't take up the whole window. I also changed the all-snippets-map-to-function test because it was only looking in quil.core for functions, but my functions are in quil.middleware. Let me know if there's something better to be done about that.

I think the PR should be ready to merge now, so I took it out of draft mode. But let me know if any changes need to be made or if anything else should be included.

[dgtized]

Hi @zsrail sorry this has been limbo for so long, but trying to move forward again with this project. For our recent release, we added quite a bit of test and release automation and finished upgrading to Processing 4.3 and p5js 1.7.0. As such, I suspect there may be some bitrot since you created this. Any chance you could try rebasing and see if it's passing the automated tests? Happy to take a look if you can't but assume you have the most context, even if that's a year old.

[zsrail]

@dgtized Thanks for trying to bring this out of limbo! I've done the rebase and all my code seems to be working fine. The automated tests for Mac and Ubuntu both crash though. Although the Mac test seem to randomly pass sometimes as well. In any case it seems like the crashes were happening even before my changes anyway.

I've also slightly rewritten the update functions in the sample programs so that they update the state map instead of just replacing it. I did that mainly so that anyone trying to experiment with navigation-2d (or any other middleware that puts things in the state map) can just add it to the middleware collection in defsketch and it will work right away.

Finally, I increased the threshold for the mouse-world-coords snippet to fail because the Clojurescript test was failing due to some aliasing on the borders of the squares, but that's not relevant to what the snippet is trying to test.

Let me know if I need to make any changes.

[zsrail]

I just pushed a new commit that addresses the effects of issue #411 on navigation-2d. Before this commit, zooming with the mouse wheel was almost completely broken on Clojurescript (at least in my browser it was). This commit also fixes a bug in the previous versions of this PR that was preventing navigation-2d from wrapping the user's draw function when using Clojurescript.

[dgtized]

Thanks for reviving this! I'm still bogged down right now as apparently in the most recent release I broke advanced CLJS compilation, so am hoping to push out a fix for that before anything else, but this would definitely be nice to release soon after.

I didn't get a chance to play with the code locally or look closely at the snippets yet but I did leave some preliminary comments.

I do really like that all of this was moved under navigation-2d in the state map. One other option here that might be worth investigating is to use namespaced keywords, ie :quil.middlewares.navigation-2d/navigation-2d for the map. I believe those are supported under CLJS but were added to the language more recently and likely would help with this particular problem of namespace collisions.

One other thing I noticed is that you added snippet-snapshots for both the normal resolution and retina. Unfortunately we aren't currently testing with retina as the linux support is broken upstream in processing or jogl for CLJ. It may be able to work with CLJS but is not currently running under automated tests. It might make sense to hold off on adding the retina expected images until after that's actually being tested?

I will come back to this again once I've fixed the CLJS issue but do let me know what you think of my comments and concerns. Looking great overall though!

[dgtized]

Is there a particular reason to cache the width/height parameters inside of the position map? I'm not sure if navigation-2d is compatible with resizing canvas or applets but I suspect this will make it trickier.

[zsrail]

The only reason was to make screen->world-coords and world->screen-coords pure functions rather than depending on some external state. I thought that would make navigation-2d more flexible, for instance allowing the user to apply it to some smaller rectangle inside their window instead of to the whole window. But you're right that it would require the user to update the width and height in the state map if the window resized, which seems annoying and redundant. Also, I guess the caching could break someone's code if they're already using navigation-2d with window resizing.

However, I've just realized that the only reason navigation-2d needs to know the dimensions of the window in the first place is because, when you zoom, it zooms in on the center of the window. If it was changed so that it zooms in on the mouse (which is what google maps does for example), then we wouldn't need to know the width and height at all.

Even if we leave the zoom centered on the center of the window, screen->world-coords and world->screen-coords could still be rewritten to not need the width and the height (and simplifying them a lot in the process). That way all the publicly exposed functions would be pure, and only the private event handlers would need to access the window's dimensions. In order to make this rewrite possible, the state map would have to store the world coordinates of the top left corner of the window (the :position key that's already there stores the world coordinates of the center of the window). I'd hesitate to change the :position key to point to the top left instead of the center since that could theoretically break someone's code if they were manually messing with :position. On the other hand, it's redundant to store both the top left corner and the center and they could become temporarily inconsistent if someone's code is updating :position manually (but no old code would be broken).

Perhaps the best thing to do is to put my changes in a separate middleware as you suggested, but I don't know what I would name it haha.

[dgtized]

While the ->> to ->> usage here is a clever way to switch the position of the threaded argument, it took me a minute to read. Maybe this would be clearer with an as->?

Secondly, I wonder if world-x, world-y and p-world-x, p-world-y might be better represented as something like a [x,y] vector named world-coords and previous-world-coords?

Alternatively, I wonder if this is better to leave to the user to use the screen->world-coords transformation on those values instead of handling this particularly transformation automatically? While it would be great to provide assistance for drag and drop, it might be better to just demonstrate this as an example but not provide automatic support.

[dgtized]

This feels odd that it's adjustable on a per frame basis instead of calculated once during sketch initialization with wrap-handlers. I kind of wonder if this firmware should be attached using something like:

:middlewares [(navigation-2d :wrap-draw false)]

Alternatively there could be two middlewares exposed by this namespace, once with draw wrapped, and other not.

Anyway, food for thought, we can go with the current approach, just explaining what about it felt strange.