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

Double shadow buffers in W3DBufferManager #604

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

xezon
Copy link
Contributor

@xezon xezon commented Feb 19, 2022

This change doubles the shadow slots in W3DBufferManager. This allows twice as many meshes in a scene to cast shadows before the buffer space runs out.

@tomsons26
Copy link
Contributor

tomsons26 commented Feb 21, 2022

When i messed with this when i wrote the buf manager initially eventually shadows started degrading and disappearing, its possible i changed something else too that caused that i guess.. So this needs careful verification of if there are regressions

Should be noted none of these changed in BFME1/2 :
MAX_IB_SIZES changed to 1024
MAX_VB_SIZES to 512
8192 in Allocate Slot Storage for VB changed to 16384

thus i suspect the issue of the shadows overflowing the buffers likely stems from the Volume Shadow classes themselves as i did also test with double values in WB when i wrote the code and it eventually throws the asserts anyway

@xezon
Copy link
Contributor Author

xezon commented Feb 21, 2022

I would say let's leave this change open for now and re-investigate as soon as more surrounding code is available. Ideally we fix it so it never crashes even if the pools depleted. I tried adding nullptr checks where the asserts hit, but that just caused the game to crash somewhere else.

Edit: Crashes are fixed with #1101

@tomsons26 tomsons26 added the investigate Investigation and careful discussion needed label Feb 21, 2022
@xezon xezon marked this pull request as draft February 23, 2022 08:25
@xezon xezon added vanilla-bug and removed bug labels Oct 17, 2022
@xezon xezon force-pushed the fix-w3d-shadow-crash branch from c7c8eb0 to 8a34c88 Compare February 10, 2024 20:25
@xezon xezon force-pushed the fix-w3d-shadow-crash branch from 8a34c88 to e97d744 Compare February 10, 2024 20:26
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cfa005f) 2.53% compared to head (e97d744) 2.53%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #604   +/-   ##
========================================
  Coverage     2.53%    2.53%           
========================================
  Files          949      949           
  Lines       110299   110299           
  Branches     18881    18881           
========================================
  Hits          2800     2800           
  Misses      107095   107095           
  Partials       404      404           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xezon
Copy link
Contributor Author

xezon commented Feb 10, 2024

I have added better comments to the shadow buffer limits.

Doubling MAX_NUMBER_SLOTS is certainly legal and achieves the desired effect. It allows for more shadows to be drawn in the scene.

It is not yet entirely clear to me if MAX_VERTEX_BUFFERS_CREATED and MAX_INDEX_BUFFERS_CREATED need doubling too, but investigation indicated so. Decreasing these buffers will have similar effect as decreasing MAX_NUMBER_SLOTS, so likely they should be scaled uniformly.

We do have a shadow test map which could be useful for testing higher limits, but this test map currently does not load correctly in Thyme because of this bug: #1053 Needs fixing first.

Should be noted none of these changed in BFME1/2 :
MAX_IB_SIZES changed to 1024
MAX_VB_SIZES to 512
8192 in Allocate Slot Storage for VB changed to 16384

@tomsons26
Can you please also check what the other constant in the other W3DBufferManager::Allocate_Slot_Storage has changed to in BFME? This may be interesting to know.

Thyme:

int vb_size = std::max(8192, size);
int ib_size = std::max(32768, size);

BFME:

int vb_size = std::max(16384, size);
int ib_size = std::max(32768, size);

@xezon xezon changed the title Avoid 3D shadow crashes by doubling buffers in W3DBufferManager Doube shadow buffers in W3DBufferManager Feb 10, 2024
@xezon xezon changed the title Doube shadow buffers in W3DBufferManager Double shadow buffers in W3DBufferManager Feb 10, 2024
@xezon
Copy link
Contributor Author

xezon commented Feb 11, 2024

BFME vs ZH

MAX_FVF          = 18                   ; same in ZH
MAX_INDEX_BUFFERS_CREATED  = 32         ; same in ZH
MAX_VERTEX_BUFFERS_CREATED  = 32        ; same in ZH
MAX_VB_SIZES     = 512                  ; 128 in ZH
MAX_IB_SIZES     = 1024                 ; 128 in ZH
MAX_NUMBER_SLOTS  = 4096                ; same in ZH
allocateSlotStorage_VB_val  = 16384     ; 8192 in ZH
allocateSlotStorage_IB_val  = 32768     ; same in ZH

@xezon
Copy link
Contributor Author

xezon commented Feb 11, 2024

We know that 512 + 1 Scorpion Tanks in the scene will deplete the shadow memory pools.

I have now tested against 1024 Scorpion Tanks in the scene.

Double MAX_NUMBER_SLOTS only with 1024 Scorpion Tanks

        MAX_NUMBER_SLOTS = 4096 * 2
        MAX_VERTEX_BUFFERS_CREATED = 32
        MAX_INDEX_BUFFERS_CREATED = 32
        MAX_VB_SIZES = 128
        MAX_IB_SIZES = 128

results in

-		g_theW3DBufferManager
		m_numEmptyVertexSlotsAllocated	4318	int
		m_numEmptyVertexBuffersAllocated	32	int
		m_numEmptyIndexSlotsAllocated	4323	int
		m_numEmptyIndexBuffersAllocated	22	int

m_numEmptyVertexBuffersAllocated now caps out at 32 and it cannot draw shadows for all meshes. m_numEmptyVertexSlotsAllocated should cap out at 8192.

Double MAX_NUMBER_SLOTS and BUFFERS_CREATED with 1024 Scorpion Tanks

        MAX_NUMBER_SLOTS = 4096 * 2
        MAX_VERTEX_BUFFERS_CREATED = 32 * 2
        MAX_INDEX_BUFFERS_CREATED = 32 * 2
        MAX_VB_SIZES = 128
        MAX_IB_SIZES = 128

results in

-		g_theW3DBufferManager
		m_numEmptyVertexSlotsAllocated	8192	int
		m_numEmptyVertexBuffersAllocated	62	int
		m_numEmptyIndexSlotsAllocated	8192	int
		m_numEmptyIndexBuffersAllocated	41	int

m_numEmptyVertexSlotsAllocated now caps out at 8192 and all meshes cast shadows in the scene

shot_20240211_095540_1

Conclusion

Scaling the following 3 limits uniformly is the correct strategy.

MAX_NUMBER_SLOTS
MAX_VERTEX_BUFFERS_CREATED
MAX_INDEX_BUFFERS_CREATED

@xezon xezon removed the investigate Investigation and careful discussion needed label Feb 11, 2024
@xezon xezon marked this pull request as ready for review February 11, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants