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.

Sign up for GitHub

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

Jump to bottom

Add a local storage of uniform constants values to the OpenGL renderer #8167

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Add a local storage of uniform constants values to the OpenGL renderer #8167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 47 additions & 3 deletions engine/graphics/src/opengl/graphics_opengl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2122,15 +2122,22 @@ static void LogFrameBufferError(GLenum status)
return ShaderDesc::LANGUAGE_GLSL_SM140;
}

static void OpenGLEnableProgram(HContext context, HProgram program)
static void OpenGLEnableProgram(HContext _context, HProgram _program)
{
glUseProgram(((OpenGLProgram*) program)->m_Id);
OpenGLProgram* program = (OpenGLProgram*) _program;
glUseProgram(program->m_Id);
CHECK_GL_ERROR;

OpenGLContext* context = (OpenGLContext*) _context;
context->m_ActiveProgram = program;
}

static void OpenGLDisableProgram(HContext context)
static void OpenGLDisableProgram(HContext _context)
{
glUseProgram(0);

OpenGLContext* context = (OpenGLContext*) _context;
context->m_ActiveProgram = 0;
}

static bool TryLinkProgram(HVertexProgram vert_program, HFragmentProgram frag_program)
Expand Down Expand Up @@ -2267,14 +2274,51 @@ static void LogFrameBufferError(GLenum status)
CHECK_GL_ERROR;
}

static bool StoreConstantValue(HContext context, const void* data, const size_t size, HUniformLocation base_location)
{
assert(size <= 64);
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we don't do defensive coding when the engine runs. While this file contains them, I'd say if's more for things like init/exit of engine and creation of the global state.

Numbers like this should generally be checked before this function. Do we even need to?

It helps us keep the engine size smaller too.


OpenGLProgram* program = (OpenGLProgram*)((OpenGLContext*)context)->m_ActiveProgram;
assert(program);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.


dmArray<OpenGLConstantValue>& values = program->m_ConstantValues;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be reset to 0, when it is disabled.
Otherwise we'll hit the memcmp() code path every time.

Which could ofc be an option to always use memcmp() to find out if it changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

E.g. something like this:

bool StoreConstantValue(in_value)
{
    ... 
    dst.changed = memcmp(dst.value, in_value) != 0;
    memcpy(dst.value, in_value, size);
    return dst.changed;
}

Also, in terms of struct size / complexity, I wonder if a simple hash32 would suffice?

E.g.:

bool StoreConstantValue(in_value)
{
    ... 
    uint32 old_checksum = dst.checksum;
    dst.checksum = dmHashBuffer32(in_value, size);
    return old_checksum != dst.checksum;
}

It takes a few extra cycles I would think, but also simplifies the logic, and saves some storage (admittedly not much)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if a simple hash32 would suffice?

I thought about that, but hash functions have a small chance of giving the same values, right? It seems to me that users can be paranoid about this. So I'll have to make an option in game.project like "optimize uniform constants". So you can toggle it on and off if you are in doubt.

int32_t location = base_location;
if (values.Size() < location + 1)
{
values.SetCapacity(location + 1);
values.SetSize(location + 1);
}

OpenGLConstantValue& value = values[location];
if (value.m_ValueSet && memcmp(value.m_Value, data, size) == 0)
{
return true;
}

memcpy(value.m_Value, data, size);
value.m_ValueSet = true;

return false;
}

static void OpenGLSetConstantV4(HContext context, const Vector4* data, int count, HUniformLocation base_location)
{
if (StoreConstantValue(context, data, count * 4 * sizeof(GLfloat), base_location))
{
return;
}

glUniform4fv(base_location, count, (const GLfloat*) data);
CHECK_GL_ERROR;
}

static void OpenGLSetConstantM4(HContext context, const Vector4* data, int count, HUniformLocation base_location)
{
if (StoreConstantValue(context, data, count * 16 * sizeof(GLfloat), base_location))
{
return;
}

glUniformMatrix4fv(base_location, count, 0, (const GLfloat*) data);
CHECK_GL_ERROR;
}
Expand Down
37 changes: 23 additions & 14 deletions engine/graphics/src/opengl/graphics_opengl_private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,28 @@ namespace dmGraphics
uint32_t m_BufferTypeFlags;
};

struct OpenglVertexAttribute
{
dmhash_t m_NameHash;
int32_t m_Location;
GLint m_Count;
GLenum m_Type;
};

struct OpenGLConstantValue
{
bool m_ValueSet;
// The maximum size of a constant is 64 bytes (4x4 matrix is the largest).
uint8_t m_Value[64];
};

struct OpenGLProgram
{
GLuint m_Id;
dmArray<OpenglVertexAttribute> m_Attributes;
dmArray<OpenGLConstantValue> m_ConstantValues;
};

struct OpenGLContext
{
OpenGLContext(const ContextParams& params);
Expand All @@ -87,6 +109,7 @@ namespace dmGraphics
WindowIconifyCallback m_WindowIconifyCallback;
void* m_WindowIconifyCallbackUserData;
PipelineState m_PipelineState;
OpenGLProgram* m_ActiveProgram;
uint32_t m_Width;
uint32_t m_Height;
uint32_t m_WindowWidth;
Expand Down Expand Up @@ -142,19 +165,5 @@ namespace dmGraphics
{
GLuint m_Id;
};

struct OpenglVertexAttribute
{
dmhash_t m_NameHash;
int32_t m_Location;
GLint m_Count;
GLenum m_Type;
};

struct OpenGLProgram
{
GLuint m_Id;
dmArray<OpenglVertexAttribute> m_Attributes;
};
}
#endif // __GRAPHICS_DEVICE_OPENGL__