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

KDE Rendering Experimental Program #804

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

JoaoDell
Copy link
Contributor

@JoaoDell JoaoDell commented Jun 15, 2023

[UPDATED]
Hello everyone, this PR contains an experimental implementation of Kernel Density Estimation calculations for a set of points. This uses some of fury's existing APIs and some straight vtk ones. This program may be a good example of shader usage and how to apply post-processing effects inside FURY. The result from running that should be:

image

Below, what is the idea here:

  1. Render billboards with KDE calculations of points to an offscreen window.
  2. Capture the offscreen window with a texture.
  3. Render a second billboard, this time to the screen, with the texture of the prior rendered window, having a post processing effect applied to it (color map application).

In more details, what is done:

  1. The offscreen and normal screen managers are created. The offscreen window is set to be for offscreen rendering with SetOffscreenRendering(True).
  2. The parameters of the rendering are created: Random points are initialized, the sigma for the KDE is set, and the scale of the billboards is defined. The points are passed to the billboard actor as its centers.
  3. shaders.shader_apply_effects() is used to tell OpenGL additive blending is desired for this code.
  4. The billboards are rendered to the offscreen window.
  5. After this render, the second billboard, the one that will receive the texture from the prior rendering, is created.
  6. The colormap is chosen from matplotlib.colormaps and passed to the billboard as a texture named "colormapTexture" with the function colormap_to_texture()
  7. The window is captured as a texture and passed to the second billboard with window_to_texture()
  8. The billboard is then finally rendered to the screen.

Some minor details that may be useful for a better understanding of it:

  • The billboard actor got a set o shaders for the KDE calculation. In its declaration, a function that calculates the non-normalized (yet, I intend to implement a normalized version) KDE as: $KDE(x, \sigma) = exp(-\frac{1}{2}(\frac{x}{\sigma})^2)$.
  • For all of that to work, I have created some functions to ease the work, functions that may be even used later in other FURY applications, so I wonder it would be a good idea to make a separate small PR for them.

@skoudoro
Copy link
Contributor

thank you for your feedback. we will look at all this links.

@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #804 (b611368) into master (3d53e48) will decrease coverage by 0.05%.
The diff coverage is n/a.

❗ Current head b611368 differs from pull request most recent head bf2b91a. Consider uploading reports for the commit bf2b91a to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #804      +/-   ##
==========================================
- Coverage   84.43%   84.38%   -0.05%     
==========================================
  Files          43       43              
  Lines       10274    10274              
  Branches     1393     1393              
==========================================
- Hits         8675     8670       -5     
- Misses       1239     1243       +4     
- Partials      360      361       +1     

see 1 file with indirect coverage changes

@JoaoDell
Copy link
Contributor Author

After a pair with @devmessias, we made some progress on the FBO setup issue. Now, the FBO is sucessfully generated, and it seems ready to be drawn to. Apparently, the issue was that the context needed to have the window interactor initialized with ShowManager.initialize() before the it was set to the FBO. I will prepare a better explaination and investigate further on why is that soon.



frag_decl = """varying vec2 out_tex;
uniform vec2 res0;
Copy link
Contributor

Choose a reason for hiding this comment

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

@JoaoDell you should declare in vec3 normalizedVertexMCVSOutput; here

frag_decl = """varying vec2 out_tex;
               in vec3 normalizedVertexMCVSOutput;
               uniform vec2 res0;
               uniform vec3 point0;"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like the problem was somewhere else, but i have just updated the PR with some changes that don't conflict with the shaders


# Render every point existing inside loop
for i in range(n_points):
shader_custom_uniforms(billboard, "fragment").SetUniform3f("point0", points[i, :])
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this action is feasible as it would attempt to create new instances of 'point0' and 'res0' variables. This leads to an error. The error message I received :

2023-06-26 19:22:22.899 (   0.328s) [        B8DAB000]   vtkShaderProgram.cxx:453    ERR| vtkShaderProgram (0x558732a3cb20): 0:109(15): error: `res0' redeclared
0:110(15): error: `point0' redeclared

We have a old PR that deals with that
#424 maybe we need to look into this again @skoudoro and @filipinascimento

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I didn't know how VTK handled uniforms so I thought the SetUniform methods only passed uniform values to the shader. That seems like something worth checking out indeed 👍

@JoaoDell
Copy link
Contributor Author

JoaoDell commented Jun 27, 2023

Hey @devmessias thanks for pointing those out, I have just updated this PR with my current progress:

  1. I could manage to stabilize the shader issues and now they seem to be working.
  2. I have cleaned better the code itself to make it simpler.
  3. I am being able to render simple colors to the billboard screen, however, I cannot seem to be able to use the color attachment as a texture after drawing to it offscreen on the FBO. When I activate the color texture to render to the screen, it outputs a pitch black render. I think that is due to my lack of enough understandment on how vtkFBOs and vtkTextureObjects work and get activated, will look better into it.

Maybe @filipinascimento could have a better idea of what is happening as well.

uniform sampler2D screenTexture;

void main(){
vec4 texture = texture(screenTexture, out_tex);
Copy link
Contributor

@devmessias devmessias Jun 27, 2023

Choose a reason for hiding this comment

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

texture should be 2D vec right?
try this

void main(){
    //vec2 texture = texture(screenTexture, out_tex);
   // gl_FragColor = texture;
  vec3 normalColor = texture2D(screenTexture, out_tex).rgb;
  gl_FragColor = vec4(normalColor.r, normalColor.g, normalColor.b, 1);
    //gl_FragColor = vec4(0.0, 0.0, 1.0, 1.0);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was passing out_tex as texture coordinates, but I will try what you recommended

@JoaoDell JoaoDell changed the title [WIP] Framebuffer Object Interface for FURY KDE Rendering Experimental Program Jul 14, 2023
@JoaoDell JoaoDell marked this pull request as ready for review July 14, 2023 17:04
@JoaoDell
Copy link
Contributor Author

Hey everyone, I just updated and completely refactored this PR as, as well as it is base program, as it went on a slight different direction in comparison to its original intetion. Everything seems to be working now, my only question is if it would be better if I closed this PR and opened a new one with a new branch containing only the recent commits, as the older ones have been completely replaced, or should I rebase it, I don't know. What should I do?

Anyway, this version is ready for review 👍

@JoaoDell JoaoDell requested a review from devmessias July 20, 2023 13:53
Copy link
Contributor

@tvcastillod tvcastillod left a comment

Choose a reason for hiding this comment

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

Hi @JoaoDell,
I need to take a deeper look at your work, but I've left some comments below. Overall is looking good.

docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ganimtron-10 ganimtron-10 left a comment

Choose a reason for hiding this comment

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

Hey @JoaoDell ,
Giving a first look I found out these below points. Some of them would require more discussion from mentors to clarify few things.
If you think any of these comments doesn't align according to your approach while implementing it feel free to ping me or mentors at the comment.

Thanks in advance!

docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
Comment on lines +11 to +21
window : window.RenderWindow,
texture_name : str,
target_actor : actor.Actor,
blending_mode : str = "None",
wrap_mode : str = "ClampToBorder",
border_color : tuple = (
0.0,
0.0,
0.0,
1.0),
interpolate : bool = True):
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure whether we use typedhint for the function parameters as we basically mention them in our docs.
Maybe @skoudoro or @filipinascimento can clarify this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not yet, but we should go toward this direction.

Actually, I think we should make it a standard for FURY and start slowly the process for all module

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so should I leave it like that and keep using typedhint for my future commits or should I stay on today's standards and not use it for now until the make it standard decision for FURY is made @skoudoro?

docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
docs/experimental/viz_points_kde.py Outdated Show resolved Hide resolved
docs/experimental/viz_points_kde.py Show resolved Hide resolved
@JoaoDell
Copy link
Contributor Author

@ganimtron-10 I have fixed and updated most of your recommendations. Indeed, typedhint isn't a FURY standard, but I will wait Filipi and Serge comments on that to change anything. Thanks for the comprehensive review!

@JoaoDell
Copy link
Contributor Author

Also, I just realised there is a native colormap generator in fury.colormp named create_colormap() so I just updated it with that.

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @JoaoDell,

Thank you for this. This experimental is much more advanced than the other experimental.

I would recommend to start adding some text and explanation and then, we can move it to the examples folder. Maybe we can create a new section named "advanced Topic".

To discuss....

Comment on lines +11 to +21
window : window.RenderWindow,
texture_name : str,
target_actor : actor.Actor,
blending_mode : str = "None",
wrap_mode : str = "ClampToBorder",
border_color : tuple = (
0.0,
0.0,
0.0,
1.0),
interpolate : bool = True):
Copy link
Contributor

Choose a reason for hiding this comment

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

Not yet, but we should go toward this direction.

Actually, I think we should make it a standard for FURY and start slowly the process for all module

@JoaoDell
Copy link
Contributor Author

Okay @skoudoro, I will be looking into the existing examples to understand how I should explain and detail this one so we can try using it as an example then, thanks for the opinion!

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.

5 participants