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

WIP Constellation plot ideas #31

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

WIP Constellation plot ideas #31

wants to merge 25 commits into from

Conversation

froyo-np
Copy link
Collaborator

@froyo-np froyo-np commented Oct 9, 2024

to run - (full instructions in examples/readme.md)

  1. pull vis repo
  2. git checkout noah/cgraph
  3. pnpm install
  4. cd examples
  5. pnpm run dev
  6. navigate to localhost://5173/constellation

@froyo-np froyo-np marked this pull request as ready for review October 17, 2024 17:04
Copy link
Collaborator

@lanesawyer lanesawyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well! I kept the review fairly light since it's not a full package yet, but I'm sure we'll use this heavily as the base for the real one. Mostly marked stray comments or TODOs.

I noticed Firefox is a bit choppier compared to Chrome if you zoom out a bit and then start dragging, but I'm fine if we do a performance pass once we start building out the real thing. (Seeing WebGL warning: drawArraysInstanced: Vertex fetch requires [different numbers but all >80k], but attribs only supply 39. in the console)

Also, an added section to the examples README would be helpful!

}
private toDataspace(px: vec2) {
const { view } = this.camera;
const o: vec2 = px;//[px[0], this.canvas.clientHeight - px[1]];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove comment

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add some controls either on the demo screen itself or in the README for the examples folder. I had to go hunting for the controls in the code to figure out how to animate it or switch the types!

const { screen, view } = this.camera;
const p = Vec2.div(delta, [this.canvas.clientWidth, this.canvas.clientHeight]);
const c = Vec2.mul(p, Box2D.size(view));
this.camera = { ...this.camera, view: Box2D.translate(view, c), screen };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks quite similar to other camera code we've had! It's probably worth making some camera state utils soonish, since most (maybe all?) of our cameras are behaving the same way.

callback: (e) => {
if (e.status === 'finished' || e.status === 'finished_synchronously') {
const stable = this.goal == this.anmParam;
// const what = goUp ? (n: number) => 1.0 - (n - Math.floor(n)) : (n: number) => n - Math.floor(n)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple comments that can probably be removed here and just below.

camera: this.camera,
Class,
Cluster,
SubClass, // TODO!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs TODOing??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todone!

// a good radius for this many cells is R where the circle with radius R has area = numCells.... maybe?
// A = pi*R*R -> sqrt(numCells/PI) = R
// then put that area in data space...
float R = sqrt(numCells/3.14159)/dotScale;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little wild that there isn't a defined PI constant readily available when writing shaders. I suppose it's because people would need different levels of specificity so it's just left as an implementation detail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is pretty wild that it isnt built in...
I also should be doing the less-lazy thing and doing
const PI = radians(180.0);
at the top of all my shaders



vec4 P = mix(p1,p2, fract(animationParam));
float dotScale = 500.0; // TODO use the radius of the dataset instead of this made-up number
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO?

target: target,
});
} else {
throw new Error('omg')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new Error('omg')
throw new Error('omg 😮')

cpuLimit?: number;
} & Omit<InnerRenderSettings, 'target'>;

// TODO: this cache key is totally insufficient!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated my comment (and I think this fn got moved)

target,

}
return beginLongRunningFrame(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use the new on here now and give a bit more battle testing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for all the great comments, I'll get to them soon I hope

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is done!

…crifice a bit of loading polish (it flickers), but the demo is simpler overall so thats ok. the practice of rendering the edges directly after all chunks of dots have been rendering is continued... its not the tidyest, but it is flexible. I could have done it in a cleaner way, but that would start to detract from the demoness of it, and I want to stop polishing this now.
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.

3 participants