Skip to content
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

Fluid.jit.plotter update - full parity with fluid.plotter #417

Open
wants to merge 77 commits into
base: main
Choose a base branch
from

Conversation

balintlaczko
Copy link
Contributor

@balintlaczko balintlaczko commented Apr 10, 2024

This is a major update for the abstraction, as it finally achieved full parity with the interface and behavior of fluid.plotter, while keeping the speed benefits.

image

Added

  • pointcolor message for setting individual point colors (parity with the same message of fluid.plotter). This can be especially useful when every point needs a unique color associated with it:
image
  • setpoint message for setting individual points, just like in fluid.plotter. There is a caveat though, since fluid.jit.plotter also has the refer interface to load large datasets super fast. To avoid accidentally modifying outside datasets, if an earlier reference was specified (via the refer message) the plot will switch to its internal dataset, making the previous set of points disappear. If you want to load a dataset and then edit its points via the setpoint message, then load the dataset first as a dictionary.

  • pointsize will now allow to change the size of individual points (minor compatibility break to the previous fluid.jit.plotter version). Also, there is no highlightedpointsize message anymore.

  • pointsizescale, that will scale all points (regular and highlighted), similar to pointsizescale in fluid.plotter.

  • highlightscale, that allows you to change the size ratio between the highlighed points and regular ones; defaults to 2.4.

Fixed

  • zooming in now respects implicit mirroring via range (bug demonstrated here)

  • decoupled object position between Edit mode and Presentation mode (bug reported here)

  • now using jit.gl.render instead of jit.pworld to render frames on-demand, instead of a constant 60 FPS. This greatly reduced idle GPU usage, which means now it is possible to have even a 100 fluid.jit.plotter instances open and sitting around without a noticable overhead. Because of the "resize watcher" script, drawing will still tick at 1 FPS on idle (so not 100% on-demand), but that did not seem to cost a lot in my tests, so I left it in there. In my experience the change also made the interaction look smoother on a high refresh rate screen.

(not sure why this PR still has commits from the previous PR, I must have messed something up, but if I can fix it, let me know, the new commits are only the ones from April this year)

- fix "let you be more draw" comment
- change speedlim to qlim to avoid the timer thread
- remove fluid.jit.plotter non-parity warning (since now there is parity)
@balintlaczko balintlaczko changed the title Fluid.jit.plotter update Fluid.jit.plotter update - full parity with fluid.plotter Apr 15, 2024
@balintlaczko balintlaczko marked this pull request as ready for review April 15, 2024 18:04
@balintlaczko
Copy link
Contributor Author

balintlaczko commented Apr 15, 2024

Okay dear people, the PR is ready for review. Please, try it and try to break it, because implementing these new stuff (especially setpoint) added a lot of complexity and new "code". On my stuff it seems to behave well. Still have to test on Windows, though, will report here when that's done.

@balintlaczko
Copy link
Contributor Author

"One more thing": I added a workaround so that the jit.gl.mesh point sizes do not change on mac os on a non-retina external screen. This is an already reported Max bug (if @benbrackenc74 is watching), which is that Max does not communicate to the render shader of jit.gl.mesh whether the screen is a retina screen or not. Mac os will report a screen resolution that is half the width and half the height to Max. With the new fix I went into the lengthy hack to check:

  • which screens are probably retina (via jit.displays: assuming that if the current resolution is 1/4th of the max resolution, we are on retina)
  • monitoring the location of the patcher that hosts fluid.jit.plotter and deducing which screen that must be
  • if we are on a mac (via gestalt) --> if the host patcher is on a "non-retina" screen --> apply a 0.5 scaling on the point sizes of the jit.gl.meshes.

This fixes the problem on my setup, I can move the patcher window (containing a fluid.jit.plotter) to the external screen and back, and the point sizes update to look the same.

N.B: this only works if the patch was first opened (or fluid.jit.plotter was created) on the retina screen, and then we move it to the external screen. Otherwise when moving from the external (non-retina) screen to the retina screen, we can observe another bug with jit.gl.mesh (also mentioned in the same bug report): that the view "zooms" to the lower left quadrant -- this is unfixable, you have to delete & undo the fluid.jit.plotter on the retina screen.

@tremblap
Copy link
Member

hmmmm and what is the behaviour between 2 types of screen without the fix? I'm super worried about the one-way-a-way-another-way-the-other-way behaviour...

@balintlaczko
Copy link
Contributor Author

The points of a jit.gl.mesh with a fixed @point_size appear larger on the non-retina screen (while let's say a jit.gl.gridshape won't change size). This means that:

  • the same settings will always lead to bigger-looking points on a non-retina screen (even if it is a 4k screen)
  • if you move the patch (or the jit.window in the example below) across screens you will see points getting bigger and smaller

Note that the red circle in the example below (that is a jit.gl.gridshape) does not change size in the process, which leads to my hypothesis that the problem lies in the shader of jit.gl.mesh not being told of the retina scaling (or something along these lines...).

output3

This bug is visible on fluid.jit.plottertoo. But with the recent hack-fix:

fluid_output

As you see the adjustment might not happen immediately, but I did not want to spend more compute resources in detecting it snappier (assuming that users won't drag windows back-and-forth all the time...).

But if you don't want this to be part of the abstraction I get that and I can take it out, or maybe not have it on by default, just let me know what you think.

@tremblap
Copy link
Member

tremblap commented May 9, 2024

Hello

I reckon this is ready to review now? One point before I do so:

But if you don't want this to be part of the abstraction I get that and I can take it out, or maybe not have it on by default, just let me know what you think.

If it takes any resource at all, let's make it at least switchable? or is the resource so low that we can ignore?

@balintlaczko
Copy link
Contributor Author

If it takes any resource at all, let's make it at least switchable? or is the resource so low that we can ignore?

I would say it consumes very little resources, but probably not nothing. Here is how it works:

  • upon object creation we create a dictionary of available screens, their coords and whether or not they are retina (assuming that if the resolution (count of pixels) if 1/4th of the reported screen resolution). This happens only once, and I haven't noticed any lag because of it.
  • then, in the js script that we use to monitor the size and position of the bpatcher in edit/presentation mode (to position the jit.pwindow acordingly) we have one more line to also report the parent patcher window's location (outlet(1, this.patcher.parentpatcher.wind.location);). This is passed into a subpatch where it is immediately gated out if the platform is not mac.
  • If it is mac, then based on the window coords we identify which screen that is, and from the dictionary we query whether it is retina or not.
  • Whenever this flag changes we apply an additional scaling to the points and refresh, once.

It sounds like a lot when I spell it out like that but I really haven't noticed any overhead. The periodic query in the js script also slows down to a 1000ms rate if it detects that nothing changes.

@tremblap
Copy link
Member

this sounds sensible. if Rod has not complained, I will test and merge like this.

@balintlaczko
Copy link
Contributor Author

Okay! Let me also test everything on Windows too.

@balintlaczko
Copy link
Contributor Author

Everything seems to be OK on Windows too!

@balintlaczko
Copy link
Contributor Author

Hold on with the merging, new update incoming (that fixes the retina stuff and does less ugly things when resizing).

@jamesb93
Copy link
Member

Did you want me to re-review this @balintlaczko ?

@balintlaczko
Copy link
Contributor Author

I think it was @tremblap who did that, but if you could hold on a bit, @rconstanzo found some potential issues that are still waiting for me to investigate. But almost there :)

@tremblap
Copy link
Member

indeed I am in standby

@rconstanzo
Copy link

rconstanzo commented Sep 30, 2024

Just found some funky edge behaviors with setpoint and related messages, the way different color points "overlap" (this happens differently in both versions (perhaps unavoidably?)), as well as the highlight -1 message (in fluid.plotter it clears stuff but throws an error in fluid.jit.plotter).

edit:
It's also mega exciting to be able to navigate large corpora with this!

@tremblap
Copy link
Member

tremblap commented Oct 9, 2024

@balintlaczko you'll let me know when you think it is ready to deep check?

@balintlaczko
Copy link
Contributor Author

Yeah, I still have some testing/fixing to do, but I'll report here when all is done!

@jamesb93
Copy link
Member

👀

@tremblap
Copy link
Member

@balintlaczko if we were to release soon, is your revision almost there or we skip a version?

@balintlaczko
Copy link
Contributor Author

Hey! Sorry, I'm still delaying the additional improvements/issues highlighted by @rconstanzo, so if you have to release very soon (in a week), then I'd say go ahead, and I'll hop on next time. Things got quite busy at work, but I'm onto the plotter fun as soon as possible, but I'll still need around 2 weeks (depending on the severity of the problems I find).

@tremblap
Copy link
Member

please do not apologize! It was just to make sure we were not 'just almost' sync but missed each other's cue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants