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

Visitor pattern in renderitem implemented in waveform #719

Closed
wants to merge 1 commit into from
Closed

Visitor pattern in renderitem implemented in waveform #719

wants to merge 1 commit into from

Conversation

mauricio790
Copy link

No description provided.

@revmischa
Copy link
Collaborator

Cool... what is it? Could you please describe what you did and why and maybe a screenshot if relevant?
Thanks for your contribution!

@kblaschke
Copy link
Member

kblaschke commented Jul 19, 2023

The whole RenderItem structure was kind of a pain in the ass, and was completely refactored in my current working branch for the 4.1 release. I kept the class, but turned it into a base class containing a few often-used OpenGL members (coordinates, VBO/VAO IDs), but the old way of adding items to a list and rendering them is gone, as it was really hard to debug, plus following the rendering order was nearly impossible.

Please have a look at the branch, which is almost ready for merging, and see if you find some potential of applying the idea there:

https://github.com/kblaschke/projectm/tree/rewrite-preset-parser/src/libprojectM

Copy link
Member

@kblaschke kblaschke left a comment

Choose a reason for hiding this comment

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

As stated above, due to the heavy rewrite of the whole rendering code, please consider looking at my working branch to see if there's still a need to implement a visitor pattern. The draft PR for this branch is also open, see #716.

Plus, as Mischa stated, please add a description to your PR describing what changes you did, what your intention was and what your change will fix/improve. I can see that from the code because I know the library internals very well, but it might not be as easy to guess for any other reviewer.

Copy link
Member

Choose a reason for hiding this comment

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

Moving Waveform-specific render logic into a generically named class probably makes the code harder to read, e.g. one wouldn't expect this code here. If there's any way to keep the code in the class with a name that actually suggests its function, please do this.

@kblaschke
Copy link
Member

I've just merged PR #716 into master, which has removed the above RenderItem ping-pong game between MilkdropPreset and Renderer. The whole Milkdrop preset rendering is now confined in the MilkdropPreset submodule, while Renderer now (mostly) contains classes supporting specific rendering objects like textures, shaders and framebuffer in an attempt to encapsulate more and more render API specific functionality in classes, so it'll be easier to port projectM to other rendering APIs like Vulkan in the future.

I'll close this PR as there's no longer a need to implement a visitor pattern for these rendering items as they're rendered in a fixed, specific order from the MilkdropPreset class.

@kblaschke kblaschke closed this Sep 15, 2023
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