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

Relative points #108

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

Relative points #108

wants to merge 12 commits into from

Conversation

mj3cheun
Copy link
Contributor

No description provided.

src/data/GraphPoints.ts Outdated Show resolved Hide resolved
@mj3cheun mj3cheun force-pushed the relative-points branch 2 times, most recently from 5da6a76 to b3d73ee Compare September 15, 2023 19:03
src/data/GraphPoints.ts Outdated Show resolved Hide resolved
import {DataTexture} from '../renderer/DataTexture';

export enum ClassModes {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

HierarchyType instead of ClassModes

fragColor = texelFetch(uPointTexture, coords, 0);

uint i = 0u;
int classIndex = texelFetch(uParentTexture, coords, 0).x;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename classIndex to parentIndex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

Comment on lines 1 to 2
#define MODE_NONE 0u
#define MODE_ADD 1u
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename this file to hierarchyType.glsl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

Copy link
Collaborator

@Nithos Nithos left a comment

Choose a reason for hiding this comment

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

Has an example been also added to the demos to see how this works?

@@ -14,6 +14,7 @@ An array of point objects to be loaded into Grafer. The PointData property list
| Property | Type | Description |
| :--- | :--- | :--- |
| id | string - *optional* | Name of the point. Will be used as an ID when referencing point in node, edge, and label data. Will default to its index in the PointData array if left out. |
| parentId | string - *optional* | ID of the point which this point is the child of. Used to enable relative positioning of points and relative radius. Will default to *No Parent* if left out.
Copy link
Collaborator

Choose a reason for hiding this comment

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

ID of the parent point that this point ...

Does it have to be a direct parent, or can it be any ancestor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the parentId put in here is the direct parent

| :--- | :--- | :--- |
| positionHierarchyType | HierarchyTypes | Changes how point hierarchies changes the point positions. See [HierarchyTypes](./hierarchy-types.md) for more information. |
| radiusHierarchyType | HierarchyTypes | Changes how point hierarchies changes the point radius. See [HierarchyTypes](./hierarchy-types.md) for more information. |
| maxHierarchyDepth | number | Sets the maximum hierarchy depth that any point will have. Defaults to 100. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

how is the depth used? why is it important? when would I want to change this to be smaller / bigger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the maxHierarchyDepth is basically only there to save the user from circular references in the data and stuff like that, which would otherwise crash the os (infinite loop in fragment shader is really not good). this should be increased if you expect the hierarchy depth in the data to be larger than the default value.

export interface PointData {
id?: number | string;
parentId?: number | string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the readme you state this as a string why and should it also be a number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh i dunno which is the correct type :(

@@ -111,28 +167,38 @@ export class GraphPoints extends DataTexture {
return this.map.get(id);
}

public getPointByIndex(index: number): [number, number, number, number] {
public getPointByIndex(index: number, isRelative = false): [number, number, number, number] {
if(isRelative) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this correct? should the old logic be run when it is relative? seems reversed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in my opinion no, because the function (the way it worked before) makes the assumption that the points returned are relative to the world space origin. this is incredibly important for stuff like tooltips etc. if we were to break this assumption we will be breaking a lot of stuff.

@mj3cheun
Copy link
Contributor Author

Has an example been also added to the demos to see how this works?

a small example was added

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