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

[FIX] Undefined behaviour on comparison of NaN and float at particlemgr.cpp #342

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

celisej567
Copy link

@celisej567 celisej567 commented Jan 2, 2025

At particlemgr.cpp in class CParticleEffectBinding there is a variable named float m_flParticleCullRadius that (as i think valve expected to happen) are 0.0f -ed in constructor by calling SetParticleCullRadius(0.0f), but looks like they didnt know that if ( m_flParticleCullRadius != flMaxParticleRadius ) that happens in SetParticleCullRadius working with float NaN, which is Undefined Behaviour, and results of this thing are declared by individual compilers realization.

As the result of the problem some particles like explosions, sparks, smoke and dust may not appear at all. There even where no drawcall at this conditions.

Its not a problem when you use same compiler version, on same system version, on same computer configuration. But it is if its a multiplatform open-source project.

Hopefully it can be fixed easely, by manually 0.0f -ing m_flParticleCullRadius in constructor.

@celisej567
Copy link
Author

celisej567 commented Jan 2, 2025

In fact you can manually execute this by doing m_flParticleCullRadius = nanf(""); instead of m_flParticleCullRadius = 0.0f;

how you can see in this example video there is no sparks from metal, no explosions and no dust from the wall.

@celisej567
Copy link
Author

celisej567 commented Jan 3, 2025

also ubuntu build script is broken i think, he cant copy any .so file that he makes

@@ -146,6 +146,8 @@ CParticleEffectBinding::CParticleEffectBinding()
m_LastMin = m_Min;
m_LastMax = m_Max;

m_flParticleCullRadius = 0.0f
Copy link
Member

Choose a reason for hiding this comment

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

This line is still missing a semicolon

Copy link
Author

Choose a reason for hiding this comment

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

look at second commit

@Blixibon Blixibon merged commit a72779a into mapbase-source:develop Jan 8, 2025
4 checks passed
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