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

OpenGL Rendering Implemented #229

Merged
merged 103 commits into from Aug 30, 2019

Conversation

@sckorpio
Copy link

commented Aug 22, 2019

Finished the OpenGL Rendering module!!!

@feragon

This comment has been minimized.

Why don't you use a destructor method here?
In that case, you don't have to call individually each destructor, they'll be automatically called by the compiler

This comment has been minimized.

Copy link
Owner Author

replied Jun 30, 2019

Last Time i tried with destructor, there were some segmentation faults. I will see this again and try

This comment has been minimized.

Copy link

replied Jun 30, 2019

You should check the order of the operations in your destructor.
It seems like calling

  VBO.~VertexBuffer();
  IBO.~IndexBuffer();
  VAO.~VertexArray();

before solved the problem. You should destroy those 3 objects at the beginning of Shape_Entity constructor.

sckorpio added 6 commits Jul 1, 2019
Update color_vertex_shader.shader
vec4 instead of vec3

@sckorpio sckorpio closed this Aug 23, 2019

@sckorpio sckorpio reopened this Aug 23, 2019

sckorpio added 2 commits Aug 23, 2019
@feragon
Copy link
Member

left a comment

It's much better, but some work is still needed

lcviewernoqt/CMakeLists.txt Outdated Show resolved Hide resolved
lcUI/CMakeLists.txt Outdated Show resolved Hide resolved
lcUI/lcadviewer.cpp Outdated Show resolved Hide resolved
lcUI/lcadviewer.cpp Outdated Show resolved Hide resolved
lcviewernoqt/painters/opengl/sources/gl_font.cpp Outdated Show resolved Hide resolved
lcviewernoqt/painters/openglpainter.cpp Outdated Show resolved Hide resolved
lcviewernoqt/viewersettings.h Outdated Show resolved Hide resolved
sckorpio added 8 commits Aug 24, 2019

@sckorpio sckorpio closed this Aug 24, 2019

@sckorpio sckorpio reopened this Aug 24, 2019

@feragon
Copy link
Member

left a comment

This is much better, but there are still some places where you left commented code and code duplication.

The fonts are still present in the PR, which means it can't be accepted because we can't redistribute those due the licenses

lcUI/lcadviewer.cpp Outdated Show resolved Hide resolved
lcUI/lcadviewer.cpp Outdated Show resolved Hide resolved
lcUI/lcadviewer.cpp Outdated Show resolved Hide resolved
lcUI/lcadviewer.h Outdated Show resolved Hide resolved
lcUI/lcadviewer.h Outdated Show resolved Hide resolved
lcviewernoqt/painters/opengl/sources/shader.cpp Outdated Show resolved Hide resolved
lcviewernoqt/painters/opengl/sources/shader.cpp Outdated Show resolved Hide resolved
lcviewernoqt/painters/opengl/sources/text_entity.cpp Outdated Show resolved Hide resolved
lcviewernoqt/painters/opengl/sources/shape_entity.cpp Outdated Show resolved Hide resolved
lcviewernoqt/painters/opengl/sources/shape_entity.cpp Outdated Show resolved Hide resolved
@feragon
Copy link
Member

left a comment

There are 3 things left:

  • Lower the OpenGL required version (3.0 would be good, unless you need a specific function from a newer API)
  • There is still one occurrence of commented code
  • You need to delete the two fonts that we can't redistribute
@@ -28,7 +28,7 @@ LCADViewer::LCADViewer(QWidget *parent) :

QSurfaceFormat format;
format.setMajorVersion(4);

This comment has been minimized.

Copy link
@feragon

feragon Aug 29, 2019

Member

For OpenGL, 3.0 means :
Major version: 3
Minor version: 0

@feragon feragon changed the base branch from master to opengl Aug 30, 2019

@feragon feragon merged commit ebbf7b9 into LibreCAD:opengl Aug 30, 2019

2 checks passed

CodeFactor 6 issues fixed. 1 issue found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.