cemyuksel / cyCodeBase

An open source programming resource intended for graphics programmers.
MIT License
271 stars 60 forks source link

Feature suggestion: replace bit masks with C Bit Fields #16

Closed neverhood311 closed 1 year ago

neverhood311 commented 2 years ago

Thanks for continuing to maintain this repository! It's been four years since I took your ray tracing class, but I find myself referring to it on a regular basis.

This is more of a feature suggestion than an issue.

This suggestion regards the bit masks in cyBVH, lines 51-57 (https://github.com/cemyuksel/cyCodeBase/blob/master/cyBVH.h#L51). Instead of relying on these bit masks to encode all of a node's data into 32 bits, why not instead use Bit Fields? They would make the BVH traversal and conversion code much easier to understand. You could replace the Node's unsigned int data with something like this:

typedef struct leafNode
{
    uint32_t isLeaf : 1;
    uint32_t elementCount : 3;
    uint32_t elementOffset : 28;
};

typedef struct internalNode
{
    uint32_t isLeaf : 1;
    uint32_t childOffset : 31;
};

union bvhNode
{
    leafNode _leaf;
    internalNode _internal;
};

I haven't done any performance tests to see whether this affects ray tracing performance, but I have tested to make sure that the bvhNode union does, in fact, take up only 32 bits (at least on my machine).

If you have a reason for continuing with bit masks, I'd like to learn what it is.

cemyuksel commented 2 years ago

That sounds like a good idea. In fact, if I were to write this class from scratch today, I probably would have done something like you suggested. I would have also replaced the dynamic array with std::vector.