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

medical: Don't divide by zero #226

Merged
merged 2 commits into from
Nov 30, 2023
Merged

medical: Don't divide by zero #226

merged 2 commits into from
Nov 30, 2023

Conversation

dastrukar
Copy link
Collaborator

Just adds a check to see if width is 0 or not.
Also, I'm not sure if this is the best/cleanest way to implement this check, but oh well.

@caligari87
Copy link
Member

I usually prefer some variation on min(), max(), or clamp() because I have a hard time parsing ternaries at a glance. I think the latter is also more expensive because it's a branching instruction? Could be wrong on that and it may or may not make a difference here anyway.

If you felt like rewriting that part thusly, it would be nice. But if not, or there's no way to do it cleanly, then this is fine and you can merge at your convenience.

@dastrukar
Copy link
Collaborator Author

So far, the only other two I can come up with is this:

		if (width > 0.) { BP.StartAlpha = min(1, max((depth / width), 0.3)); }
		else { BP.StartAlpha = 0.3; }

and this

		BP.StartAlpha = min(1, max(depth / (width + 0.00001), 0.3));

i think this should make it more consistent with the code?
@dastrukar
Copy link
Collaborator Author

Changed it to the "if else" check, since that probably makes more sense than the "add 0.00001 to prevent div by zero" and also more readable than the ternary thing.

@caligari87 caligari87 merged commit 83fbf1a into master Nov 30, 2023
@dastrukar dastrukar deleted the fix-divbyzero_width branch December 15, 2023 16:40
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