Skip to content

Conversation

@illwieckz
Copy link
Member

@illwieckz illwieckz commented Sep 30, 2025

Use VectorNormalizeFast() instead of VectorNormalize() anytime the returned length isn't used.

It does the same, but with less computations since we don't need to compute the length.

Copy link
Member

@slipher slipher left a comment

Choose a reason for hiding this comment

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

This one causes a bug. See the dark triangle on the upper right:

Image

tr.sunLight[ 2 ] = atof( token );

VectorNormalize( tr.sunLight );
VectorNormalizeFast( tr.sunLight );
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any point to using the fast version for something that's done very infrequently and only at loading time.

@slipher
Copy link
Member

slipher commented Sep 30, 2025

The bug demonstrated above affects a BSP surface, so I assume it is something that happened at load time. Probably it would be better to avoid using the "fast" version for anything during load and only use it for hot paths that happen every frame.

@illwieckz
Copy link
Member Author

Interesting… So VectorNormalizeFast() does something different outside of not returning the length?

@slipher
Copy link
Member

slipher commented Sep 30, 2025

I would expect VectorNormalizeFast to have weaker accuracy guarantees.

@illwieckz
Copy link
Member Author

Ah yes we now use Q_rsqrt_fast() instead of Q_rsqrt() in it.

@illwieckz
Copy link
Member Author

Interesting:

unvanquished_2025-10-01_012500_000

unvanquished_2025-10-01_012513_000

@illwieckz
Copy link
Member Author

I orgot I added a faster-less-accurate rsqrt in VectorNormalizeFast() so yes we better reserve this for hot paths.

@illwieckz illwieckz closed this Sep 30, 2025
@illwieckz illwieckz deleted the illwieckz/vectornormalizefast branch September 30, 2025 23:36
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