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

Use MemoryStack api for OpenGL and document it for SimpleDrawElements #24

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Bawbby
Copy link

@Bawbby Bawbby commented Oct 20, 2018

No description provided.


import static org.lwjgl.glfw.GLFW.*;
import static org.lwjgl.opengl.GL11.*;
import static org.lwjgl.opengl.GL15.*;
import static org.lwjgl.system.MemoryUtil.*;

import java.nio.FloatBuffer;
Copy link
Member

Choose a reason for hiding this comment

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

Why change the import order?

Copy link
Author

Choose a reason for hiding this comment

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

My mistake, I moved the imports by accident I'll fix that.

@@ -37,7 +39,7 @@ public void run() {
glfwDestroyWindow(window);
keyCallback.free();
wsCallback.free();
if (debugProc != null)
if ( debugProc != null )
Copy link
Member

Choose a reason for hiding this comment

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

Why change formatting?

Copy link
Author

@Bawbby Bawbby Oct 20, 2018

Choose a reason for hiding this comment

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

The code uses both with spaces and no spaces between parens, I just changed formatting to make it consistent. I can revert it if needed.

@@ -81,7 +83,7 @@ public void invoke(long window, int key, int scancode, int action, int mods) {
glfwSetWindowSizeCallback(window, wsCallback = new GLFWWindowSizeCallback() {
@Override
public void invoke(long window, int w, int h) {
if (w > 0 && h > 0) {
if ( w > 0 && h > 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

Formatting?

@@ -90,7 +92,7 @@ public void invoke(long window, int w, int h) {

GLFWVidMode vidmode = glfwGetVideoMode(glfwGetPrimaryMonitor());
glfwSetWindowPos(window, (vidmode.width() - width) / 2, (vidmode.height() - height) / 2);
try (MemoryStack frame = MemoryStack.stackPush()) {
try ( MemoryStack frame = MemoryStack.stackPush() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Formatting.

// Load the data into buffers and send it off to OpenGL using the MemoryStack api
// instead of BufferUtils
// Note: Don't forget to flip any buffers before passing them into gl functions that need them
try ( MemoryStack stack = MemoryStack.stackPush() ) {
Copy link
Member

Choose a reason for hiding this comment

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

I think using the MemoryStack for buffer data upload is stretching it a bit. In this example, since it is only 3 vertices it's okay, but it might give the wrong idea to people. The problem is that MemoryStack is not meant for potentially large buffer data, which one usually has when uploading models/vertices. A better alternative for this is manual Buffer memory management with memAlloc/memFree.

Copy link
Author

@Bawbby Bawbby Oct 20, 2018

Choose a reason for hiding this comment

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

I had forgotten to state why I used the MemoryStack vs other methods. I'll add a note about using different methods for allocating buffers and the reasons why.

@httpdigest httpdigest force-pushed the master branch 2 times, most recently from 0342101 to c20d13e Compare March 2, 2019 11:11
@httpdigest httpdigest force-pushed the master branch 15 times, most recently from b402ed6 to 475b266 Compare May 26, 2019 11:54
@httpdigest httpdigest force-pushed the master branch 16 times, most recently from 3a903d2 to a95a3f2 Compare November 2, 2019 23:01
@httpdigest httpdigest changed the base branch from master to main September 23, 2020 07:57
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.

2 participants