patriciogonzalezvivo / glslViewer

Console-based GLSL Sandbox for 2D/3D shaders
BSD 3-Clause "New" or "Revised" License
4.62k stars 350 forks source link

fix: (ADA) Prefer smart pointers over raw pointers #276

Closed tcoyvwac closed 2 years ago

tcoyvwac commented 2 years ago

Hi @patriciogonzalezvivo,

As the project is a C++11 project, this PR helps replace some raw pointers with smart pointers to help prevent unknown memory leaks. _(For example: possibly m_plot_texture in sandbox.{cpp,h}.)_


(This is a twin PR, but for the ADA branch of the project, as somewhat suggested in PR:https://github.com/patriciogonzalezvivo/glslViewer/pull/276#issuecomment-1195533985)

patriciogonzalezvivo commented 2 years ago

Wow! this is fantastic! Thank you! There is a small caveat. I'm working on a different branch to replace ADA for VERA

VERA is more ambitious than ADA (which I will probably later prone to a simpler library) it have a app.h and ops/draw.h API that mimics P5js which GlslViewer don't really use, but a scene definition GlslViewer extend for the global uniform def.

All the geometry loaders for PLY, OBJ, GLTF and STL were moved to VERA together with other geometry operations from HILMA

Why migrating from ADA to VERA? One of VERA's goal will be to bring native support for a big variety of surfaces just like ADA but focusing more heavily on VR/AR/MR through WebXR support.

VERA

All this to ask you if you would be interested to make this similar changes on VERA and VERA's GlslViewer branch ??

tcoyvwac commented 2 years ago

Hi, thanks for the feedback and update @patriciogonzalezvivo! That sounds awesome; it is a very exciting vision & I love the upcoming changes! :smile_cat:


Humm, could you help me with my re-summary of the development situation (for understanding): :mag_right:

tcoyvwac commented 2 years ago

Adjusted the commit so the changeset is now shared as a twin PR between the ADA and VERA branches.

tcoyvwac commented 2 years ago

Ok @patriciogonzalezvivo, these twin PRs are basically finished.

If there are no extra requests or recommendations, feel free to merge please! :smile_cat:

tcoyvwac commented 2 years ago

@patriciogonzalezvivo can you merge the ADA PRs into your project's mainline please ? :smile_cat: :+1:

ADA branch seems more "specification-complete" than the VERA branch currently right now. I am still understanding your codebase and the ADA branch is what I use to compare my code to (my local tests), and to understand & see if any of my code-fixes & features breaks the codebase.


For example:

pcl_poisson_fill:
    glslViewer pcl.ply pcl.vert pcl_poisson_fill.frag iphone_depth.jpeg -e camera_position,0.8877,-0.228562,-1.34835 -l

On the ADA branch, this passes & has always worked with no problems :heavy_check_mark: , however, on the VERA branch, this is (past and) currently not working / cannot verify correctly. :heavy_multiplication_x:


I am regularly comparing the code in your code-repo, between the both branches at the same time together, and strongly need them (at upstream) to be in sync (to help understanding your codebase / analysis). :mag_right:

tcoyvwac commented 2 years ago

This PR is now out-of-date so closing. :+1: