Turtle enhancements #404
Turtle enhancements #404
Conversation
|
I have changed my opinions on my parameter handling methods. I'm converting to draft. |
|
Things we still need to fix that have to do with parameter handling:
|
|
Right now turtle just keeps drawing and drawing more of the path by default, but it will eventually get laggy if you don't pause it. Should we do something about that? |
|
Sorry for the git mess, I rebased onto ui2 properly this time but I probably didn't do it optimally. |
|
After the rebase, a change in behaviour: when you change one of the NUMBER_ARRAY parameters (domain, turns, etc) it restarts the animation but doesn't take into account the update. however, if you hit the restart button on the header, it will restrart WITH taking into account the update. |
I suspect somehow that the issue here is that we are checking if a parameter changed, but arrays values are by reference, so maybe the reference hasn't changed? |
Do you mean that the time to draw each individual additional stroke increases? That seems strange --can you post a URL for which that happens? |
This only applies when we're doing the folding animation, which requires redrawing the path each frame. Take for example a random walk with folding turned on: http://localhost:5173/?name=Random&viz=Turtle&steps=1+1+1+1+1&walkAnimation=true&growth=100&startEndControl=true&foldAnimation=true&folding=100&seq=Random If you increase the turtle speed on that one (growth) it really gets messy |
|
I'm seeing really weird behaviour on this example: http://localhost:5173/?name=Beatty+forever&viz=Turtle&modulus=3&domain=0+1+2&turns=79+0+45&steps=1+1+1&walkAnimation=true&growth=100&startEndControl=true&start=0+200&folding=0+10+0&bgColor=e9eee3&strokeColor=4b7a81&seq=Formula&formula=floor%28n*sqrt%282%29%29 ... if I use the browser reload button vs. the refresh button on the header, I see a different picture... but reliably repeatable. I get one picture when reloading browser and a different when using refresh button. Repeatable with different browsers, and after restarting numberscope. Thoughts? |
Wow, that is really unfortunate. Will make that my next priority after admin stuff (closing issues, etc) post-Delft-code-merges. |
|
But maybe you could rebase this code in the meantime on ui2? We could even cross our fingers and hope it improves the situation... Let me know when it is synced up with ui2. |
katestange
force-pushed
the
turtle-enhance
branch
from
01bb387
to
9315fdc
Compare
|
Just an update, in case anyone is worrying about what's going on here. I rebased and force pushed, but actually the tests don't pass. Then I worked on fixing the merge issues and got to the point the code will run, and committed despite tests not fully passing yet. I know the idea is only to commit things that work, but the rebase broke lots of things. So my rationale here is that each commit here is strictly improving matters, and I want to commit some things to stay sane. So I'll keep going until I get back in a state where all tests pass. |
|
Of course that's OK under the circumstances. And I also want to make sure you know that if the behavior of the Turtle visualizer has legitimately changed in such a way that the expected output of one of the previously existing tests is now different, you can get the tests to pass by changing the expected snapshot to match the new behavior. There is even a command-line flag for doing this snapshot copying; it's |
|
PS. just wanted to point out that in your latest additions to math.ts, the new functions say in their comments that they return |
Thanks. Actually I just realized, what's the desired behaviour for math.mul(posInfinity,0n) for example? |
We are not doing full blown ieee-style arithmetic here with a separate "Not a Number". Since 0n×♾️ is indeterminate and the range of "potential values" includes Infinity, just return Infinity. All indeterminates should return either ±Infinity, since we have no other values to represent them. Also don't sweat this too much because the switch over to mathjs v13 should move all of these issues into mathjs, not our code. |
|
The most recent push has us back to a functioning turtle after rebase in the most important respects, but there are still bugs and issues and the e2e tests are failing. I'm not confident enough in it to take new snapshots for the tests yet. |
|
One question: I was doing a try...except to catch CachingError before but now lint objects to this -- are we not throwing CachingErrors anywhere anymore, or did I mix up when changing the imports? Not sure why that's suddenly showing up. |
|
Second question: The P5GL panning is working (yay!) but the wheel zoom is not. I need to run some turtle code (to clear canvas) upon wheel events, but the mouseWheel() function that I'm inheriting complains it wants an event input. We had to add mouseWheel() to P5Visualizer.ts at one point, but of course P5GL is paying attention to these events and maybe something is getting interrupted. Not sure if you have a suggestion here, or if I'm using the P5GL controls wrong. |
|
Another question related to debugging. I'm seeing weird behaviour when I change the number of terms in the sequence pane. Which stuff gets re-run when that changes? |
It's the only occurrence of the identifier |
OK, I just pushed a commit to your branch that improves this situation a little bit. Make sure you pull it and incorporate it into your working tree (you may need to stash your changes before the pull). But it does not fix it altogether. To see that wheel zoom is doing something, go to a drawing turtle where you can see what's going on, and roll your mouse wheel rapidly a moderate distance and then left-drag (pan) the view immediately afterward. You should see the whole drawing zoom in or out. But if you just mouse wheel gently, the event seems to be getting lost; and if you don't pan, it seems as though what's already been drawn doesn't get refreshed. I am actually not sure what is going on here. A single frame of Turtle is super quick, there's almost nothing to do. So maybe this is some kind of race condition between the event handling and the draw()? But if so, why does panning work so nicely (and fix the problem with zooming)? And why does zoom work just fine in Histogram? Do you want to poke around about this in p5 webgl posts online, or do you want me to debug it when I next have time? Whichever you would enjoy more ;-) |
As far as the visualizer knows, it's a completely new sequence. So everything that gets run on a reload should get re-run. Is there something that seems not to be getting run that you want it to run? Or conversely, is something being re-run that you don't think should be? |
|
Thanks for the responses, that fixed some stuff! I'll keep mucking about. |
Wow and whatever it was you did, the zooming seems to be working fine now =) Not quite sure how you did that, but it's really nice to have pan and zoom for Turtle. I will also say, that since you're now drawing with WebGL, it's actually drawing in 3D, so there's no practical reason not to have a 3D turtle and re-enable the "orbit" control as well. But maybe you'd feel that 3D Turtle should be a separate visualizer? I am kind of thinking that we should put it in either as an extra panel you can pop up in this one or as another closely related visualizer that shares code. I feel like it would be good to go with because I am sure there are some sequences and recipes that will look very good in 3D, and I think it will have "wow factor" when we do release... |
There's a Wow factor to just reading this comment! But first let's get the 2d working reliably! |
|
Yes, I do agree, first things first. And 3D turtle graphics is noticeably more intricate than 2D: In 2D, just forward and turn suffice, but in 3D, you need at least forward, yaw (that's the equivalent of 2D turn), and either pitch or roll; it's probably easiest just to allow either/both pitch and roll. Here's hoping that some nice sequences with relatively simple 3D turtle instructions produce knotted paths ;-) |
|
I just added some commits that address all the remaining issues raised in Glen's last-week list except "_Turtle still has the default behavior that when the screen is resized, it redraws from the beginning." In particular, I've done some aesthetic improvement of gallery specimens, added a separate "uniform fold rate" option, added a bit in the test docs about updating snapshots (since it is coming up for me), removed the start position/bearing, loosened warnings, small code adjustments. |
|
One issue I'm seeing is that VFib Snowflake featured e2e test is flaky. It seems like maybe it's capturing one fewer/more frame sometimes? Another issue that has happened a few times is that --update-snapshots doesn't actually produce all the snapshots needed. This time it only made ThueTrellis-chromium (and not firefox). Earlier it happened and Glen noticed some snapshots were missing and fixed it. I don't know why that happens?? |
|
Glen, I'm having a tough time finding some time to wrap this up, but it's very close. If you could adjust the resize issue, that would be great, then I'd be ok with marking it ready for review. Otherwise I'll try to find time soon. |
Yes, I will add a commit or so that prevents Turtle from restarting on resize. |
Yes, I agree. It seems like it is doing something like that. My personal feeling is to live with it (unless it becomes intolerable) until the new improved view controls are implemented, since they will likely solve the issue. If they don't, then I'd say we debug it then.
My personal theory is this is connected with the flakiness and so my attitude/approach is the same. |
|
One other thing: Did you not like my suggestion of replicating the last entry of any of the vectors of step parameters (length, turn angles, fold, stretch) if there are more elements in the domain than in the vector? This would seem to pretty much cover the use case for "uniform folding rate" (just specify one fold, and it gets used over and over) and also things like '8 120 0 0 0 0 0' -- that could be just '8 120 0'. If you don't object, I will implement this, remove uniform folding rate, and adjust all of the gallery specimens to take advantage of this ability. Thanks for letting me know your views on this. |
OK, I am busily eating these words. (Nom, nom.) I simply could not get VFib or ThueTrellis to reproduce the snapshots you recently pushed, so I was forced to look into this. It turns out that when the URL requests "frames=64" in Thue Trellis and I load the page, I'm currently really only getting a varying amount of frames with mode 52 (and it's not hard for me to imagine your modal number of frames is different). The problem seems to be that somehow the specimen initializes itself seven (count them, 7!) times, with one or two calls to draw() each time that end up getting wiped out by the next initialization. So there is clearly something wrong going on at frontscope startup, which I am trying to debug. Wish me luck, I will need it. I fear the inscrutable internals of Vue and/or p5 are about to need some serious scruting. ;-) |
|
OK, it seems to be entirely caused by (A) the Vue reactive mechanism "fooling" our code into thinking that parameters had changed when they had not, and (B) the fact that when some parameters change (simultaneously, e.g. when loading a URL) we call the "parameterChanged" callback on the visualizer once for every parameter, potentially causing it to reinitialize itself on each call. I have already put in a safeguard against (A) in my local version of the code -- if a parameter seems to have changed, I convert its old and new values back to their string formats and only consider it a change if those are different. But for (B), on reflection, I cannot understand why the Delft team made multiple calls to parameterChanged when they also wrote code that meant each call to parameterChanged would by default reset the visualization. That seems a recipe for doing unnecessary multiple resets. I'm beat right now, but my plan is tomorrow to change this callback method to parametersChanged, which is always called just once, but with a nonempty list of the parameters that appear to have changed rather than just one parameter name. Fingers crossed that will get us to where we produce more consistent results. I am hopeful because in my working copy where I just short-circuited the call, I indeed get a full 64 frames of drawing on every page reload. Will keep y'all posted. |
Wow, thanks Glen! (And your "nom nom" made me laugh!) I guess this is another example of the end to end tests being valuable (which is apparently equivalent to causing lots of extra work!). |
Well, I guess I'm torn. Recall we've talked about the "naive user" vs. "power user" issues. Your suggestion seemed a bit like a "power user" feature, since the naive user is unlikely to read the descriptions of the parameters in detail and notice they can do this. But then I thought, this is actually a very nice feature to have accessible and even default, and putting it up front makes it available to the naive user. So while I have no objection to changing the default handling of "too short" folding arrays (repeat last entry instead of zero out remaining entries does seem more useful and so I agree this should change actually), and I recognize the redundancy you are pointing out, I'm now a bit fond of having an up-front way to turn on animation easily for the naive user. Is there a solution which makes this "uniform fold" an easy default for the naive user while still being fairly clear that you can do individual folds and making it easy for power users? I guess we could change the default folding array parameter to just one entry, and leave the individual folding for power users to discover by reading the parameter description in more detail. But then maybe the functionality is too hidden? I don't see a super obvious best solution. |
Vue reactivity and parameter updating were interacting poorly. This commit
protects against this bad interaction in two ways:
1. Only changes to values that have different string forms than the
previous value. This ensures, for example, that a reactive value is not
modified just because it is not `===` to the apparently "new" value.
2. Changes the `parameterChanged()` method to be called once for each
parameter change to `parametersChanged()` so that multiple simultaneous
changes are bundled into one call.
With these changes, the startup sequence is more predictable. We hope this
change will make end-to-end testing significantly more reproducible.
Also makes Turtle not restart drawing from the beginning of the path on
a resize, since it is now able to redraw the full path so far without
difficulty.
Also fixes the default value for the `highlight` parameter of FactorFence;
the multiple initializations were masking the prior erroneous value.
|
All right, cleaning up the initialization and preventing rampant re-running of "assignParameters" indeed seems to have made the testing much more robust. Turtle is now not restarting the path because of a resize. On the rule parameters: Since we agreed that the last value should be reused as needed, made that change. Since that technically made the "uniform fold rate" redundant, I removed that as well. And then as a compromise, I documented everything as though in any of the rule parameters (turns, steps, folds, stretches) you should either put a single value or a list positionally corresponding to the domain. Both of those things work pretty intuitively, and I hope that it ended up relatively clear and not too complicated/forbidding for an initial user. These are the two main, natural ways of using the parameters, anyway. (And in fact, you get some nice pictures in VFib Snowflake if you set the turn angle to a constant and just have the step lengths depending on the residue, so a fixed number for all steps is useful even in the turn angle parameter.) Take a look at let me know if you're comfortable trying it like this. To me it seems like you can do most things you'd want to, without it being so overly complicated to start with. I am going to assume that this is ready for review, and start reviewing it as if. If that's the case, please mark as ready for review; if there are other things you know of that you want done (whether you have the time to or not), please let me know. |
|
Added a rough check of domain appropriateness, to address #76. |
|
Added an error for the sake of #412. |
|
OK, I feel like I've gone through this pretty thoroughly and I am not seeing anything else of concern. So I think the Turtle overhaul is done (for now anyway, see #471 ;-) ). Yay! Merging. |
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 PR provides enhancements to the turtle visualizer:
Note: This turtle visualizer is not backward compatible to past URLs.
Things to consider implementing in a subsequent PR:
As I find some more nice examples, posting a few links here:
http://localhost:5173/?name=Uncoiling+Dragon+2&viz=Turtle&modulus=2&domain=0+1&turns=20+1690.04&steps=20+100&growth=10&startEndControl=true&firstTerm=1&pathLength=10000&foldAnimation=true&folding=30+0&bgColor=37674c&strokeColor=b5cfb4&seq=OEIS+A002260