Remove unnecessary payload data_size duplication
#2575
Conversation
The `InitDx12AccelerationStructureCommandHeader` meta command header already contains the "uncompressed data size" as a field, it is just named `inputs_data_size` instead of `data_size`. Because of this, an additonal `data_size` field has been added to the associated API Payload structure instead of simply renaming the field. I propose to remove this additional `data_size` field and rename `inputs_data_size` into `data_size` to be more coherent with the meta command dispatching framework Change-Id: I3a6608fca0000e208603e5f9b7d0918ae2bd0ab4
|
CI gfxreconstruct build queued with queue ID 605439. |
|
CI gfxreconstruct build # 8397 running. |
|
CI gfxreconstruct build # 8397 passed. |
|
The error looks like a network error unrelated to my commit, can I get a confirmation ? |
davidd-lunarg
left a comment
davidd-lunarg
left a comment
There was a problem hiding this comment.
This looks good to me. I do have one comment/question. @jzulauf-lunarg Could you also review?
| { | ||
| const char* label = "init DX12 acceleration structure meta-data block"; | ||
| ParameterReadResult read_result = ReadParameterBuffer(label, block_buffer, header.inputs_data_size); | ||
| ParameterReadResult read_result = ReadParameterBuffer(label, block_buffer, header.data_size); |
There was a problem hiding this comment.
The third argument uncompressed_size to BlockParser::ReadParameterBuffer is ignored when compression is off, so header.data_size (previously header.inputs_data_size) is not used to set result.uncompressed_size.
gfxreconstruct/framework/decode/block_parser.cpp
Lines 259 to 263 in aae6ac7
Looking at how block_buffer.Remainder() is calculated, I think uncompressed_size == block_buffer.Remainder() when the uncompressed_size argument is non-default. @jzulauf-lunarg Should an assert be added in ReadParameterBuffer to enforce/verify this? For example:
else
{
// For uncompressed blocks the data size is the parameter buffer size
+ GFXRECON_ASSERT(uncompressed_size == kReadSizeFromBuffer || uncompressed_size == block_buffer.Remainder());
result.uncompressed_size = block_buffer.Remainder();
}
3 participants
The
InitDx12AccelerationStructureCommandHeadermeta command header already contains the "uncompressed data size" as a field, it is just namedinputs_data_sizeinstead ofdata_size.Because of this, an additonal
data_sizefield has been added to the associated API Payload structure instead of simply renaming the field.I propose to remove this additional
data_sizefield and renameinputs_data_sizeintodata_sizeto be more coherent with the meta command dispatching framework.