search

BredaUniversityGames / DXX-Raytracer

DXX Raytracer is a fork of the DXX Retro project for Windows. DXX Raytracer uses modern raytracing techniques to update the graphics of the beloved retro game known as Descent.
Other
216 stars 10 forks source link

Fix for incorrect normal direction calculation for overlay textures #6

Closed Lehm2000 closed 10 months ago

Lehm2000 commented 10 months ago

Fix for incorrect normal direction calculation for overlay textures by rotating triangle tangents by face orientation (to match rotated uv coordinates). Also moved interpolating normal and tangent data into GetHitMaterialAndUVs and added interpolated data as return values.

Contingencyy commented 10 months ago

The only thing I would ask you to do is to change the switch statement into the proposed arithmetic solution of picking the right amount to rotate. Otherwise this looks good to me.

Lehm2000 commented 10 months ago

Good idea. Updated the PR to replace the switch with the power of maths and also changed it to now only calculate rotated tangents if they are needed. Original commit had it calculating it for every pixel regardless of if it was a base texture or a not rotated overlay.

Contingencyy commented 10 months ago

Seems okay, the only thing is we have removed one dynamic branch and introduced another, but one with potentially less variance between threads. So it does not really fix what I asked, but I doubt it will have any noticeable performance benefits in this case. Once I found a second reviewer, we can merge this, or we might adjust the merge rules. Also quick side note for GPU programming, because this might be quite un-intuitive if you only ever programmed for CPUs before: Doing seemingly useless extra work even though you don't need to rotate an orientation of 0, and instead introducing a dynamic branch might actually end up slower, which is due to the way that GPUs run code on individual threads and use something that is called SIMT (single instruction multiple threads) and how each thread in a warp/wave executes code. I don't think its gonna make a noticeable difference in this case, because neighboring pixels are gonna take the same path most likely, but its important to keep in mind for GPU programming in general. This is of course a pretty high-level explanation, I am not sure what your experience level with GPU programming is, so feel free to research more if interested.

Lehm2000 commented 10 months ago

Yes, I am more familiar with CPU programming, but your explanation makes sense. If you want me to remove that if statement, more than willing to. I want it to be code you are happy with.

Contingencyy commented 10 months ago

As I said, I think its fine in this case. If you do not notice a difference in frame time between your version and the previous one, it should be just fine. I will also verify that before merging. I just wanted to clarify why I wanted the change to happen, and also hopefully drop some useful knowledge. I specifically recall one instance where a dynamic branch cost >1ms, so its definitely important to verify and profile.

Lehm2000 commented 10 months ago

With and without the if statement, the frame time was basically the same (~12.9ms on my machine) looking at the wall with the energy center decal from the same spot on level 1.