glslViewer icon indicating copy to clipboard operation
glslViewer copied to clipboard

fix: (ADA) Prefer smart pointers over raw pointers

Open tcoyvwac opened this issue 1 year ago • 5 comments

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)

tcoyvwac avatar Jul 26 '22 13:07 tcoyvwac

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 ??

patriciogonzalezvivo avatar Jul 26 '22 14:07 patriciogonzalezvivo

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:

  • You are replacing the current ADA module / codebase with the codebase from the VERA project. This conversion is happening on branch VERA.

  • A project called HILMA is also being merged into VERA / branch VERA?

  • This current PR & changeset will still be merged into the ADA version of this project, however:

    • it is encouraged that all future PRs be directed to the VERA branch? Or...
    • it is requested to be twin PRs / changes, be made at the same time for both the ADA branch and VERA branch?

tcoyvwac avatar Jul 27 '22 16:07 tcoyvwac

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

tcoyvwac avatar Jul 29 '22 14:07 tcoyvwac

Ok @patriciogonzalezvivo, these twin PRs are basically finished.

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

tcoyvwac avatar Jul 31 '22 16:07 tcoyvwac

@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 avatar Aug 01 '22 16:08 tcoyvwac

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

tcoyvwac avatar Aug 25 '22 16:08 tcoyvwac