-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feature/tree map vis #66
Conversation
…work Feature/visualization framework
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output looks really nice, well done ^^
As a general remark. There are still a lot of commented lines of code and console.log statements present in the tree map code. I believe that some code cleanup is in order.
It's a pity though that ellipses came too late for the demo ;-;
let width = Math.abs(bounds.right - bounds.left); | ||
let height = Math.abs(bounds.top - bounds.bottom); | ||
|
||
// TODO: fix performance issues related to drawing BOTH a fill ánd outline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance should be fine for this, however memory I estimate to reach about 10GB with the current OpenGL implementation. I plan to take care of that in #64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see how to get around doing a Math.abs() if I need to get a width/height from 2 points which could both be negative. Is Math.abs relatively heavy then?
The only alternative I see is to start working with if/else statements, but that sounds to me as re-implementing Math.abs()
.
If you have a suggestion I'd love to hear it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Math.abs is about as cheap as a math function can be. My comment was added to line 45.
|
||
var childOrientation = ''; | ||
// console.log(orientation, (orientation==='HORIZONTAL')); | ||
if (orientation === 'HORIZONTAL') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use expensive strings for this? I'm not sure if enums exist in Typescript, but I would use that if they exist. Or at the very least make these class level readonly constants so that new strings don't get created all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I'm lazy and wanted to do something enum-like, but couldn't be arsed to look up how to do this in practice. I'll try and revise later this weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enums do exist, you can check my PR for forms
top = parentBounds.top - parentHeight * doneSize / parentSize; | ||
} | ||
|
||
const newBounds = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to return this immediately without first storing it in a local variable. Not that big of an issue though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, will update. It's not often that I had to think about performance in the past.
this.colorB[3] - this.colorA[3] | ||
]; | ||
this.totalNodes = tree.subTreeSize; | ||
this.offset = 2; // TODO: implement as parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to make an issue for this. Similar to how enhancements for the Pythagoras tree were handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With regards to the color, yes I think so as well. The offset is tree-map specific, though I don't think you're referring to line 34 with your comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring to line 34 since I thought that it was a visualization setting :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#71 is now a thing.
gl.fillAAQuad(100, 100, 100, 100, [0, 1, 0, 1]); | ||
gl.drawAAQuad(100, -200, 100, 100, [0, 1, 0, 1]); | ||
gl.fillLinedAAQuad(-200, -200, 100, 100, [0, 1, 0, 1], [0, 0, 0, 1]); | ||
gl.drawRotatedQuad(-200, 100, 100, 100, 45, [0, 1, 0, 1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is intended behavior, but it's seriously bugging me out that it isn't aligned with the other quads 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️
I should probably also add that I actually managed to render the entire NCBI dataset on my TU/e laptop. But performance wasn't acceptable and Firefox kept asking me to kill a tab that was using a lot of resources. |
Thank you for the feedback @RoanH . I thought I'd do the cleanup at a later stage as there are quite some changes coming up to this in the future I believe. Though I understand the wish to keep the codebase clean. I'll revise and update with a new commit somewhere later this sunday. When I'm at it, I'll also add in circles to the demo visualization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very nice! Well done. My only remark is that you remove the console.log calls, but as @RoanH already mentioned this, I approve 👍
Also improved the code by adding some comments.
UpdateDemo visualizationAs can also be seen in #55 there is now an update to the openGL demo visualization which includes the addition of circles and ellipses to that visualization. Tree-MapThe following changes were made:
|
Update from develop
Update OpenGL demo visualizationThe demo visualization now supports a few settings to play around with! Set the color on a 0 to 255 scale per channel, and toggle the sine waves on or off! Update treemap visualizationThe treemap visualization now support a first user-setting as well! Namely, offset. This creates the effect of nested treemaps. |
@bartwesselink I think the CI thingy doesn't like the properties missing which I posted about on the telegram group. Unfortunately I can't see the details of the check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
] | ||
this.sinewaves = settings.sinewaves; // update whether we should draw the sine waves | ||
|
||
this.gl.releaseBuffers(); // remove old data from buffers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like something a lower level component should probably handle, the visualisation framework for example. There are also better ways to update the OpenGL state without clearing all the buffers, but those would have to be implemented first so for now this is indeed the only way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would imagine a partial buffer update then?
But this seemed like a straightforward approach which just worked
.
We can come back to this later then, I'll post an issue, if there isn't one already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we'll also have to see to what extent an implementation for partial updates would be desired (since it would also add some overhead). For example the tree map vis wouldn't really benefit from it as changing the offset invalidates just about all the coordinates.
I'm aware of the offset thing. It flips the quads as lowerbound-edges become upperbound-edges and visa versa. However, as I'm already way past executing the Thanks for the input though! @bartwesselink Thanks for implementing and changing it to a slider, and fixing the errors! |
Overview
fixes #55
fixes #48
OpenGL demo visualization
To provide a visual demo of the openGL methods and a visual guide (the red centered-cross) indicating how our axis are aligned. The cross has a domain and range of [-200, 200] with ticks every 10 units. The addition of this visualization can prove a decent starting point for those who want to get started with creating their own visualization. In the future this visualization can be modified to suit our needs better as more features are added to the tool in general. (i.e. we could also -soon- showcase basic usage of parameters in this visualization as well when #59 is merged)
Additionally I made the demo visualization the first visualization to be loaded, as that seems like a reasonable thing to do at this stage.
The demo visualization
Treemap implementation
I had to do research on the tree-map, and whilst there are many variations, I opted to start with the most straight-forward implementation. I know the task originally only said to research, but determining the algorithm for the tree-map is something I found easier to do whilst implementing it immediately. Hence we now have an extra visualization implemented in a basic form.
As a minor feature there is also a 2-color gradient implemented, but that works better when the
offset
is also implemented later on. This is because a non-nested treemap visualizes the leaves, which have colorA in a colorA -> colorB gradient. A common use-case for color in a treemap which could perhaps be better, is to do it based on some property of the node, e.g. file-extension in a filesystem.the treemap visualization
Problems / issues
The treemap visually works better if there is both an outline ánd a fill. This however means that on a hierarchy of 300.000 items, there are 600.000 calls to draw a rectangle on screen.
Hence large-hierarchies on a laptop system are not yet supported, and untested on a better system.
What also popped up is that by default my laptop would render using the internal IntelHD graphics chip, this caused performance to be bad, even when only drawing the filled quads. (Hence having only 300.000 elements, not 600.000). Forcing my laptop to use the gtx1050m it has gave rise to major improvements in terms of speed. However, my system has 16GB of memory, and it can't manage drawing the 600.000 elements with either GPU without causing the
openGL context
to belost
.Perhaps something to discuss in a different issue/topic/thread.
Future todo
As visualizations are very stand-alone, I would propose to merge this
as is
right now. However, the treemap visualization aspect of this PR is still open to some improvements which, due to the modular nature of visualizations, I would opt to push forward a bit in a future issue/PR.The following items I would like to improve upon in the future / separate issues: