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

[test] ofTexture more straightforward way to see if it support GL_RGB16F #8212

Merged
merged 28 commits into from
Dec 8, 2024

Conversation

dimitre
Copy link
Member

@dimitre dimitre commented Nov 25, 2024

No description provided.

@dimitre dimitre marked this pull request as ready for review December 5, 2024 17:17
@dimitre
Copy link
Member Author

dimitre commented Dec 5, 2024

@ofTheo I think this one is good to go. it fixes things for RPI and completes compilation on aarch64.
it reveals apothecary or system libraries issues (GLEW) on previous RPI versions

glInternalFormat = GL_RGB16F;
#else
glInternalFormat = GL_RGB;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Feels like this would maybe be a safer change as it keeps the original logic, but also checks for the define

		if(ofGLESVersionFromGL() >= 300) {
				#if defined(GL_RGB16F)
						glInternalFormat = GL_RGB16F;
				#else
						glInternalFormat = GL_RGB;
				#endif
			textureTarget = GL_TEXTURE_2D;
		} else {
			glInternalFormat = GL_RGB;
			textureTarget = GL_TEXTURE_2D;
		}

Copy link
Member Author

Choose a reason for hiding this comment

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

My main idea was depart from ofGLESVersionFromGL which is very imprecise.
if you read the function it tries to parse from a string (that greatly varies from platform, video card) and I am suspecting this function returns both false positives and negatives.

a more deterministic way of define would be get this info from glGetString(GL_SHADING_LANGUAGE_VERSION);

but the simpler way would be just checking the presence of the define. if it exists it can be set.

OF uses this approach a lot in ofCubeMap

			#if defined(GL_RGBA16F)

cc: @NickHardeman

Copy link
Member

Choose a reason for hiding this comment

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

yeah - I think I was just imagining possible regressions where it might now default to GL_RGB16F instead of GL_RGB - one is a floating point texture and the other is unsigned char.

So that could mean some shaders might behave a little differently - curious if that would be very noticeable or not really an issue.

I think in general bug fixes should correct breaking bugs or existing issues without changing behavior, hence my caution.

@ofTheo
Copy link
Member

ofTheo commented Dec 5, 2024

Thanks @dimitre - unrelated curious why we still have the armv7l CI checks - thought we dropped armv7 builds?

@dimitre
Copy link
Member Author

dimitre commented Dec 5, 2024

@ofTheo I've put back working runners and fixed cross compiling for armv6l and armv7l about 7 months ago.
They were running great. now they are missing GLEW so I'm not sure if we were using apothecary ones or via apt.
we talked about dropping but there were other ideas, like 32 bits and rpi zero (or something like this)

@ofTheo
Copy link
Member

ofTheo commented Dec 8, 2024

@dimitre can you double check / fix the apothecary changes that are pulled into this PR and if it all looks good feel free to merge?

@dimitre
Copy link
Member Author

dimitre commented Dec 8, 2024

@ofTheo I've cleaned up this PR to reflect the needed changes, so it is good to go in my opinion.
it can potentially help with emscripten issues

@ofTheo ofTheo merged commit af33e91 into openframeworks:master Dec 8, 2024
6 of 15 checks passed
@ofTheo
Copy link
Member

ofTheo commented Dec 8, 2024

Amazing - thanks @dimitre !

@dimitre dimitre deleted the tex branch December 9, 2024 18:53
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