Skip to content

gl: add profile-neutral GLSL dispatch and core test support - #65

Open
tritao wants to merge 6 commits into
stack/egl-offscreenfrom
stack/profile-neutral-shaders
Open

gl: add profile-neutral GLSL dispatch and core test support#65
tritao wants to merge 6 commits into
stack/egl-offscreenfrom
stack/profile-neutral-shaders

Conversation

@tritao

@tritao tritao commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

Make Coin's GLSL implementation independent of compatibility-profile entry points and establish reusable core-profile GL test support.

  • Add profile-neutral GLSL dispatch with core entry points and ARB fallbacks.
  • Query extensions safely when compatibility-only extension-string access is unavailable.
  • Migrate shader objects, programs, parameters, and stage nodes to the neutral dispatch layer.
  • Add GLSLRuntimeTest coverage for shader compilation, linking, uniforms, and execution in a core-profile context.
  • Add shared hidden-GLFW context and framebuffer support under testsuite/support/ for generic GL tests.
  • Expose the small generic GL dispatch surface needed by the shared test support.

This layer does not own EGL display/context lifecycle or caller-state restoration; that remains in PR #12. Shader diagnostic/source-attribution behavior is intentionally left to PR #22.

This PR replaces #14, which GitHub recorded as merged while the active renderer stack was being repaired. The implementation remains on the existing stack/profile-neutral-shaders branch so later layers retain their original ancestry.

Stack

Layer 3/18, based on PR #12. PR #22 is the next layer.

@tritao tritao changed the title stack/profile neutral shaders gl: add profile-neutral GLSL dispatch and core test support Aug 18, 2026
@tritao
tritao force-pushed the stack/profile-neutral-shaders branch from ca87a6a to a6f4a6d Compare August 18, 2026 19:51
@tritao
tritao force-pushed the stack/profile-neutral-shaders branch 2 times, most recently from 59b3a2c to e5bdbba Compare August 19, 2026 01:16
@tritao
tritao force-pushed the stack/profile-neutral-shaders branch 2 times, most recently from acffc99 to b57eac0 Compare August 19, 2026 02:14
Comment thread CMakeLists.txt Outdated

option(COIN_BUILD_SHARED_LIBS "Build shared library when ON (default), static when OFF." ON)
option(COIN_BUILD_TESTS "Build unit tests when ON (default), skips them when OFF." ON)
option(COIN_FETCH_TEST_DEPENDENCIES

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer respecting the FreeCAD policy of not pulling dependencies using the build system but requiring them externally using find_package. This adds "superbuild" complexity that is not actually necessary

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. FetchContent is removed. Coin uses , with dependencies provisioned externally through native package managers.

Comment thread src/glue/glslp.h Outdated
#define COIN_GLUE_GLSLP_H

/**************************************************************************\
* Copyright (c) Kongsberg Oil & Gas Technologies AS

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

may not be a question for you but do we need to keep the Kongsberg copyright for new files?

Comment thread src/glue/glslp.h

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what is 'p' supposed to mean in the filename?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It means private I think, it's a previously existing Coin convention.

Comment thread src/glue/gl.cpp Outdated
Comment on lines +2688 to +2697
cc_glglue_glGetUniformLocation(const cc_glglue * glue, GLuint program,
const char * name)
{
if (glue->glGetUniformLocation) return glue->glGetUniformLocation(program, name);
if (glue->glGetUniformLocationARB) {
return glue->glGetUniformLocationARB((COIN_GLhandle) program,
(const COIN_GLchar *) name);
}
return -1;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

move impl to glslp.cpp? this file is already long enough

Comment thread src/glue/gl.cpp Outdated
}


/* Standard-facing shader wrappers. Keeping the fallback here means shader

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

standard-facing?

Comment thread testsuite/CMakeLists.txt Outdated
Comment on lines +126 to +128
${PROJECT_SOURCE_DIR}/src
${PROJECT_SOURCE_DIR}/include
${PROJECT_BINARY_DIR}/include

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would be surprised that these directories are not included when you link the test executable target with Coin

Comment thread testsuite/CMakeLists.txt Outdated
Comment on lines +139 to +143
target_include_directories(GLSLRuntimeTest PRIVATE
${PROJECT_SOURCE_DIR}/src
${PROJECT_SOURCE_DIR}/include
${PROJECT_BINARY_DIR}/include
${COIN_TARGET_INCLUDE_DIRECTORIES})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

redundant with public includes from CoinGLTestSupport, this should not be necessary

Comment thread testsuite/GLSLRuntimeTest.cpp Outdated
Comment on lines +11 to +21
int skip(const char * reason)
{
std::cout << "SKIP: " << reason << std::endl;
return 77;
}

bool check(bool condition, const char * message)
{
if (!condition) std::cerr << "FAIL: " << message << std::endl;
return condition;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

at this point, we can think about a small header for test utilities defining classic CHECK, SKIP macros

Comment thread testsuite/support/GLTestFramebuffer.h Outdated
std::vector<uint8_t> readPixels() const;

private:
const cc_glglue * glue_;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is suffixing with a _ common in this code base? I don't see the point

int width() const { return width_; }
int height() const { return height_; }

std::vector<uint8_t> readPixels() const;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

doc at least the pixel order format

@tritao
tritao force-pushed the stack/profile-neutral-shaders branch from b57eac0 to 564a6c8 Compare August 19, 2026 10:22
@tritao
tritao force-pushed the stack/profile-neutral-shaders branch from 3a1313c to 02d5615 Compare August 19, 2026 10:28
@tritao
tritao force-pushed the stack/profile-neutral-shaders branch 2 times, most recently from e92e352 to 3ddbee8 Compare August 19, 2026 12:08
@tritao

tritao commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. I rewrote the PR around the feedback:

  • Moved GLSL dispatch into its own private implementation.
  • Added explicit core/ARB availability checking.
  • Simplified the shared GLFW test support and CMake linkage.
  • Removed FetchContent in favor of external, native dependency provisioning.

@tritao
tritao force-pushed the stack/profile-neutral-shaders branch 4 times, most recently from 2cefee8 to 61a4642 Compare August 19, 2026 13:16
@tritao
tritao force-pushed the stack/profile-neutral-shaders branch from 61a4642 to a3891c3 Compare August 19, 2026 13:56
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