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

Document fog functions Gfx_SetFog(2) and Play_SetFog #1922

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

Conversation

Thar0
Copy link
Contributor

@Thar0 Thar0 commented Mar 8, 2024

Finally an explanation for the weird SPFogFactor values.

Thanks Wiseguy for pointing out the overflow issue when near and far are close

// Calculating the fog multiplier when far = 1000 and near >= 997 using `128000 / (far - near)` as in
// SPFogPosition would result in an overflow since 128000 / (1000 - 997) > 32767. Avoid this overflow by
// effectively clamping near to ~996. This is almost SPFogPosition(996.0937f, 1000)
gSPFogFactor(gfx++, 0x7FFF, -0x7F00); // Fixed point: 0x7FFF = 1.0, -0x7F00 = -0.992
Copy link
Collaborator

Choose a reason for hiding this comment

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

fm and fo are s0.15 ? Where is this from
Seems inconsistent with the formula
fogAlpha=fm*z+fo z in [-1;1] fogAlpha in [0;255]
(cf gbi.h)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f(z) = clamp(32767 * z - 32512, 0, 255) for z in [-1,1] produces the curve (only the interesting part shown)

image

32512 is chosen so that f(z)=255 at z=1

There's some weirdness in how the microcode actually computes this since z itself isn't really in the interval [-1,1], it's in s15.16 format. The calculation is something like

vmudh $v__, fo, [1,1,1,1,1,1,1,1] // ACC = (fo * [1,1,1,1,1,1,1,1]) << 16
vmadh $v__, [0, 0, 0, 1, 0, 0, 0, 1], v31_0x7F00 // ACC += ([0,0,0,1,0,0,0,1] * 0x7F00) << 16
vmadn $v26, pos, fm // frac. part, $v26 = low16(ACC += pos * fm)
vmadh $v25, pos, fm // int. part, $v25 = mid16(ACC += (pos * fm) << 16)

https://github.com/Mr-Wiseguy/f3dex2/blob/master/f3dex2.s#L1432
I have yet to take the time to carefully check the units involved here

Copy link
Collaborator

Choose a reason for hiding this comment

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

then if to interpret 0x7FFF and the other as s0.15 you rely on interpreting z as s15.16 that should be written somewhere too, otherwise it's kinda arbitrary/useless to use any unit

Gfx* Gfx_SetFog(Gfx* gfx, s32 r, s32 g, s32 b, s32 a, s32 near, s32 far) {
// Ensure fog far is greater than near
Copy link
Contributor

Choose a reason for hiding this comment

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

this only ensures that near != far, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it ensures that far is greater than near only in the case where they are initially the same. like, the important bit that tharo is stressing here is that far gets incremented by 1 to be bigger.
but its prob good to clarify that in the wording

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Ensure fog far is greater than near
// Avoid 0 divisor in gSPFogPosition below

Comment on lines +177 to +178
* Set the default fog state controlled by the environment system.
* If a custom fog state is used in rendering it is expected that it be reset back to this default state.
Copy link
Collaborator

@Dragorn421 Dragorn421 Mar 10, 2024

Choose a reason for hiding this comment

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

forgive me but I find this comment somewhat clunky, I tried to rewrite it more to my liking

 * Set the environment fog, from parameters controlled by the environment system.
 * If a custom fog state is momentarily used in drawing, the environment fog is expected to be restored afterwards.

* Set fog color and range.
*
* At or prior to fog near, geometry is unaffected by fog. At or beyond fog far, geometry is fully fogged.
* Between near and far pixels will be linearly interpolated between the unfogged color and the supplied fog color.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Between near and far pixels will be linearly interpolated between the unfogged color and the supplied fog color.
* Between near and far the geometry color will be interpolated between the unfogged color and the supplied fog color.

it is "some z"-linear but not view space z linear when using perspective projection (I forgot the details (if I ever grasped them)), anyway so I don't think specifying linear is all that useful

Gfx* Gfx_SetFog(Gfx* gfx, s32 r, s32 g, s32 b, s32 a, s32 near, s32 far) {
// Ensure fog far is greater than near
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Ensure fog far is greater than near
// Avoid 0 divisor in gSPFogPosition below

gDPSetFogColor(gfx++, r, g, b, a);

if (near >= 1000) {
// Fog far is expected to be at most 1000 so near >= far. Set a constant shade alpha of 0 for no fog
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Fog far is expected to be at most 1000 so near >= far. Set a constant shade alpha of 0 for no fog
// Set a constant shade alpha of 0 for no fog

I wouldn't write anything on why, this whole function is sloppy without good "why"s anyway

} else if (near < 0) {
// Fog near is out of range. Set a constant shade alpha of 255 for fully fogged
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Fog near is out of range. Set a constant shade alpha of 255 for fully fogged
// Set a constant shade alpha of 255 for fully fogged

Comment on lines +882 to +883
//! @bug If the difference between near and far is not at least 4, the fog multiplier will overflow. This is
//! handled above when fog far is 1000 but for closer fog far it is not accounted for.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
//! @bug If the difference between near and far is not at least 4, the fog multiplier will overflow. This is
//! handled above when fog far is 1000 but for closer fog far it is not accounted for.
//! @bug If far - near < 4, the computed `fm` fog factor coefficient will overflow.
//! For example: 128000 / (983 - 980) > 32767
//! This is handled above in the case of near > 996, but the general case is not accounted for.

Comment on lines +873 to +875
// Calculating the fog multiplier when far = 1000 and near >= 997 using `128000 / (far - near)` as in
// SPFogPosition would result in an overflow since 128000 / (1000 - 997) > 32767. Avoid this overflow by
// effectively clamping near to ~996. This is almost SPFogPosition(996.0937f, 1000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest moving details to the general case else { below and making this part lighter

Suggested change
// Calculating the fog multiplier when far = 1000 and near >= 997 using `128000 / (far - near)` as in
// SPFogPosition would result in an overflow since 128000 / (1000 - 997) > 32767. Avoid this overflow by
// effectively clamping near to ~996. This is almost SPFogPosition(996.0937f, 1000)
// Avoid an overflow when near and far are close (see bug below), by effectively clamping near to ~996.
// This is almost SPFogPosition(996.0937f, 1000)

@fig02 fig02 added the Waiting for author There are requested changes that have not been addressed label Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs second approval Waiting for author There are requested changes that have not been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants