-
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
Generalized Pythagoras Tree #63
Conversation
…ta structure Rough skeleton of the generalized pythagoras tree. The core of the algorithm and its form are there but some functionality should be split in to seperate functions and some functions lack their required logic still. There is also a rough data structure for storing a tree. It's currently inefficient and not fully featured. However, it was required to have one. Can be adapted or exchanged for one that is more suited to the rest of the project.
…rithm NOT YET TESTED
…tive to old position
…rge to main branch first
Looks really nice! I'll review it when I have some more time. Also you might want to implement some better coloring for the squares. If so then I recommend a HSB Gradient since they are really easy to compute and look really good. The general formula for these gradients would be: return Color.HSBtoRGB(subTreeSize / 256.0F, 1.0F, subTreeSize / (subTreeSize + 8.0F)); That's in Java of course, but it should be quite easy to translate. Also I took this snippet from my fractal generator so you might have to lower the 256. In any case, this looks really good. Edit: Using the height of a node from the root would give better results. |
Also a quick question, why does the diff of this PR show the entire OpenGL documentation being removed and added again with seemingly no changes? |
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! As @RoanH mentioned, does this still require a gradient?
|
||
// Define the base rectangle of the visualized tree, these values can later be determined in settings | ||
// Rectangle is an array of 5 numbers, in order: center x, center y, width (X), height (Y), angle | ||
let rectangle = [0, -250, 50, 50, 0]; |
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.
For future PRs, it is good practise to store these values in variables, preferable static, readonly ones, such that they can easily be edited. Considering the fact that settings still have to be implemented, I believe this is not a must now.
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.
Yes I agree with you, but this was indeed left this way because it is going to be changed very soon for the settings anyway. However I will change it now since I am changing some other variables anyway :)
|
||
weightHeight = 1; // !! Currently broken !! Scaling factor for the height of every rectangle that is not the root |
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.
Please place these values (with access modifier public/protected/private) at the top, imo it looks nicer and is cleaner.
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.
Forgot about that! Normally I only quickly place values like these for testing purposes, but it slipped my mind this time.
private generate(tree: Node, rectangle: number[], gl: OpenGL): void { | ||
// Draw the previously calculated rectangle | ||
gl.fillRotatedQuad(rectangle[0], rectangle[1], rectangle[2], rectangle[3], rectangle[4] * 57.295779513082320876798154814105, this.color); |
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.
Please modify this number to 180/Math.pi . I think that's cleaner. If this is because of performance, then please create a value radToDegreeConstant or something like that.
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.
Will do. I try to have the least amount of "magic numbers" as possible, but forgot to change this one in to a constant.
let rectangleChild = [center[0], center[1], width, height, rotation]; | ||
// Finally we recurse using all the previously calculated values | ||
this.generate(element, rectangleChild, gl); |
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.
Is it possible to check whether the current value of the size is +- 1, and then just stop?
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.
Sorry, I don't understand what you mean by this. What size and where? And stop what? The loop or the recursion?
On line 34 I already check to see if the node has any children. Is that what you mean?
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.
Ah, I admit it is a bit vague. I mean that a certain depth there is no point in rendering a square because the size of it will be between 0 - 1 pixels. This is not visible to the human eye, so maybe it is good to check whether it makes sense to continue the recursion.
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.
Great remark! This slipped my mind somehow since thinking about it a couple days ago. Something like this should probably be implemented, but I'm not sure how that would interact with regards to zooming. Similarly for the future it might also be a good idea to not render outside the viewport, but then there is perhaps a similar problem with panning.
Also we currently view the canvas as having a size of 1600x900, but as @RoanH mentioned in actuality it is not drawn on a pixel grid.
I feel like the complexity of this issue is above the current PR and would require more thinking and discussion about the implementation, inner workings of OpenGL and future requirements.
So I could implement a simple check (width < 1, height < 1 for example) but this might end up causing other issues which I can not for see right now.
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.
If you ask me performance is actually preatty good. I could visualise the NCBI dataset in it's entirety in well under 10 seconds on the TU/e laptop. It's also worth noting that OpenGL already ignores vertices that are not visible and that OpenGL shaders run per pixel (and therefore don't run at all if the size is under a pixel or they only run for a single pixel).
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.
One thing that we can do though is tell the OpenGL layer to ignore drawing shapes with no width/height all together. We need to have them computed for zooming and panning purposes though. I also have a few OpenGL optimizations in mind that when successful will reduce memory usage by over 16 times.
@RoanH possibly line endings? |
I have no clue why the OpenGL documentation shows as being added and removed. As mentioned I even compared it to the one in develop and could not find any differences. Git also does not detect any changes when exchanging the file from develop in to this branch. Edit: |
The error looks like there is no data uploaded yet, and I get a similar error if I try to create a render without uploading the data first. Is the tree view on the right showing the data correctly? |
Yeah that is indeed the case. The things was that I was expecting the default view to update when I uploaded a data set, but that's not how it works. I had to create a new tab. Everything seems to work now though so I'll do a full review in a bit. |
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 good and performance is also quite good already.
The NCBI dataset also works just fine and response times are acceptable, 3.8GB of total memory usage is also acceptable for now in my opinion (tested on a TU/e laptop).
I'd say that this can be merged, especially so since enhancement issues have already been created and can be addressed at a later date (so we can focus on more important things first).
let rectangleChild = [center[0], center[1], width, height, rotation]; | ||
// Finally we recurse using all the previously calculated values | ||
this.generate(element, rectangleChild, gl); |
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.
If you ask me performance is actually preatty good. I could visualise the NCBI dataset in it's entirety in well under 10 seconds on the TU/e laptop. It's also worth noting that OpenGL already ignores vertices that are not visible and that OpenGL shaders run per pixel (and therefore don't run at all if the size is under a pixel or they only run for a single pixel).
let rectangleChild = [center[0], center[1], width, height, rotation]; | ||
// Finally we recurse using all the previously calculated values | ||
this.generate(element, rectangleChild, gl); |
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.
One thing that we can do though is tell the OpenGL layer to ignore drawing shapes with no width/height all together. We need to have them computed for zooming and panning purposes though. I also have a few OpenGL optimizations in mind that when successful will reduce memory usage by over 16 times.
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 good, is documented, and performance is very reasonable. Further changes/enhancements to this could be handled in future issues, especially due to the modular nature of visualizations.
I discussed and the items mentioned in the review are either handled, or part of optimization. The latter shall be handled in a possible future issue if it is still relevant then. Additionaly, the visualizations are modular in nature, and hence this merge only adds functionality, without impacting other facets of the tool. Hence I dismiss this review for now so the PR can be merged.
Full implementation of the Generalized Pythagoras Tree, implemented within the visualization framework. This mean it can be called simply using
draw(Node, OpenGL)
. The rendering is done recursively and most of the code was implementing and figuring out the correct formulas. Hopefully the comments in the code help explain some of the steps.The pull request for some reason shows that the documentaton.md has changes, but I did not change anything within this file. I suspect that it is an artifact of solving the merge conflicts, however when comparing the file locally to the one currently in the develop branch there are no differences to be found. If possible somebody else should double check this.
If you have any questions please let me know.
To-do:
Math.sin
andMath.cos
calls. There are also some other simple optimizations which I do not believe cost a lot of computational time. If possible I would prefer to tackle some of these at the same time.Known bugs:
Screenshots:
(A,B,C):
((A,D),(C,B)):
newick_example_phyloviz.nwk: