Skip to content

Conversation

@riccardobl
Copy link
Member

No description provided.

@pspeed42
Copy link
Contributor

pspeed42 commented Aug 8, 2023

Note regarding the protected void isValidNumber(float v) methods.

...because they are protected it is harder for Java to inline them but even if asserts are turned off they will be called.

Either way, I wonder if it would be better if isValidNumber() simply returned true/false and then the calls are replaced with asserts... then when asserts are turned off no extra calls are made.

Else, isValidNumber() should be renamed because "is" implies a boolean return.

@riccardobl
Copy link
Member Author

I've updated the pr accordingly to your suggestions.
I would add the content of the invalid uniforms in the assert message eg.
assert isValidNumber(c) : "Invalid Quaternion value: " + c;
but i am not sure if this can result in performance degradation even if assertions are disabled.
Any thoughts?

@pspeed42
Copy link
Contributor

pspeed42 commented Aug 8, 2023

My understanding is that if assertions are disabled then the entire part after "assert" is removed... including the string concatenation.

@pspeed42
Copy link
Contributor

pspeed42 commented Aug 8, 2023

...and by the way, I think this kind of instrumenting is really great for those "special times" when it's necessary.

@riccardobl
Copy link
Member Author

I added the values to the assert messages

@riccardobl riccardobl added bug Something that is supposed to work, but doesn't. More severe than a "defect". enhancement labels Aug 9, 2023
@stephengold stephengold linked an issue Aug 9, 2023 that may be closed by this pull request
@riccardobl
Copy link
Member Author

If there aren't objections i will merge this

@capdevon
Copy link
Contributor

capdevon commented Aug 17, 2023

Hi @riccardobl, please take a look at my revision before merging the code, there are code formatting errors, extra lines and double semicolons at the end of the instructions. Could you please correct them?

@riccardobl
Copy link
Member Author

Hi @riccardobl, please take a look at my revision before merging the code, there are code formatting errors, extra lines and double semicolons at the end of the instructions. Could you please correct them?

Mhh i don't see your revision anywhere, but i think i've fixed the issues you pointed out, see the last commit

@Ali-RS
Copy link
Member

Ali-RS commented Aug 18, 2023

Looks like he forgot to "submit" the reviews, so they won't be displayed to the public.
This also happened to me before!

@capdevon
Copy link
Contributor

capdevon commented Aug 19, 2023

Looks like he forgot to "submit" the reviews, so they won't be displayed to the public. This also happened to me before!

@Ali-RS Correct, I have submitted the reviews which should now be visible.
Thanks for your patience @riccardobl . I think there is still a little something to correct ;)

@riccardobl
Copy link
Member Author

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something that is supposed to work, but doesn't. More severe than a "defect". enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PointLight.setRadius(POSITIVE_INFINITY) silently breaks the engine

5 participants