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

Added support for bvecX not operator #1347

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

Conversation

KaruroChori
Copy link

Added support for the missing not operator (only for boolean vectors).

@ZXShady
Copy link

ZXShady commented Mar 4, 2025

Is this needed? we have glm::not_ which follows GLSL specification

@KaruroChori
Copy link
Author

KaruroChori commented Mar 4, 2025

I think so? https://en.cppreference.com/w/cpp/language/operator_logical
and or and not are valid replacements for && || and ! under the C++ standard specs.
Not having the not operator working artificially makes valid glsl not compatible in C++.

For example, this is valid with my patch applied, else not:

if( all(cond) || all(not(cond)) ) s*=-1.0;

So yes, I would say having it is a good idea in place of glm::not_ unless I am missing something.

@ZXShady
Copy link

ZXShady commented Mar 4, 2025

I think so? https://en.cppreference.com/w/cpp/language/operator_logical
and or and not are valid replacements for && || and ! under the C++ standard specs.

they are second class they are alternatives.

Not having the not operator working artificially makes valid glsl not compatible in C++.

For example, this is valid with my patch applied, else not:

if( all(cond) || all(not(cond)) ) s*=-1.0;

So yes, I would say having it is a good idea in place of glm::not_ unless I am missing something.

I don't think glm allows exact copy pasting of code into C++ due to language differences. glm tries to

and the fix is just this

> if( all(cond) || all(not_(cond)) ) s*=-1.0;

notice the _ in not_ and now it compiles.

also with your patch code like this compiles

if(any(not vec))

and if we are following GLSL spec then this code should not compile.

I don't think it is important we have GLSL not function instead of ! being overloaded for vectors in both GLSL and GLM.

also having 2 functions for the same thing is weird, have one or the other and GLM uses GLSL not function instead.

tldr just use glm::not_ (note the _ because not is a reserved keyword)

@KaruroChori
Copy link
Author

KaruroChori commented Mar 4, 2025

Sorry but I am really having a hard time following your points.

they are second class they are alternatives.

Even if, then what?
not is the reserved keyword used as alternative for !. The semantic of such operator is logical negation.
The library currently offers component-based overloads for both or and and, so it does make perfect sense to support the unary not.
Even excluding gsls-related considerations about code compatibility, it does simply make sense to have an idiomatic C++ implementation.

I will agree that some of the digraphs are not often used in the modern world of unicode, but I don't quite get where your "second class" label is coming from to be honest. They are there, the standard describes them as fully interchangeable, people use them, so...

I don't think glm allows exact copy pasting of code into C++ due to language differences.

Sure, I agree, but why artificially introducing some when it is not needed?

notice the _ in not_ and now it compiles.

Yes, but why would anyone able to choose pick the underscore in place of same code?

and if we are following GLSL spec then this code should not compile.

Valid point, but I see two problems there:

  1. Personally, I think in general it is more acceptable to have C++ code extending glsl, compared to having glsl features not matching.
  2. What really matters is that it is not a regression introduced by my code, it already existed before.

From func_vector_relational.inl around line 57

	template<length_t L, qualifier Q>
	GLM_FUNC_QUALIFIER GLM_CONSTEXPR bool any(vec<L, bool, Q> const& v)
	{
		bool Result = false;
		for(length_t i = 0; i < L; ++i)
			Result = Result || v[i];
		return Result;
	}

If we want it removed, a different PR might do that (alongside any other invalid overload which has been defined).

also having 2 functions for the same thing is weird

I agree, not_ should be set as deprecated and scheduled for removal in some far point in the future in my opinion.

@ZXShady
Copy link

ZXShady commented Mar 4, 2025

You are correct, indeed && and || are not defined in GLSL, so this makes sense to be added as well, since glm already extends the vector class

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