Hello folks! Given the method VoxelBuffer:

Correcting your example, should be 2*padding: <div class="highlight highlight-sour

So the correct way should be: <div class="highlight highlight-source-c++ notransla

Fixed in <a class="commit-link" data-hovercard-type="commit" data-hovercard-url="https

Understanding VoxelBuffer::copy_from fail condition about godot_voxel HOT 8 CLOSED

zylann commented on August 16, 2024

Understanding VoxelBuffer::copy_from fail condition

from godot_voxel.

Comments (8)

Zylann commented on August 16, 2024 1

Oh. Yeah that condition should be the opposite. Looks like I never used this function either xD Thanks for pointing these mistakes!

from godot_voxel.

afonsolage commented on August 16, 2024

There is a similar problem (I think so) on other VoxelBuffer::copy_from. Given:

godot_voxel/voxel_buffer.cpp

Lines 213 to 260 in 7ced53b

 void VoxelBuffer::copy_from(const VoxelBuffer &other, Vector3i src_min, Vector3i src_max, Vector3i dst_min, unsigned int channel_index) { 

 ERR_FAIL_INDEX(channel_index, MAX_CHANNELS); 

 Channel &channel = _channels[channel_index]; 

 const Channel &other_channel = other._channels[channel_index]; 

 Vector3i::sort_min_max(src_min, src_max); 

 src_min.clamp_to(Vector3i(0, 0, 0), other._size); 

 src_max.clamp_to(Vector3i(0, 0, 0), other._size + Vector3i(1, 1, 1)); 

 dst_min.clamp_to(Vector3i(0, 0, 0), _size); 

 Vector3i area_size = src_max - src_min; 

 //Vector3i dst_max = dst_min + area_size; 

 if (area_size == _size) { 

 copy_from(other, channel_index); 

 } else { 

 if (other_channel.data) { 

 if (channel.data == NULL) { 

 create_channel(channel_index, _size, channel.defval); 

 } 

 // Copy row by row 

 Vector3i pos; 

 for (pos.z = 0; pos.z < area_size.z; ++pos.z) { 

 for (pos.x = 0; pos.x < area_size.x; ++pos.x) { 

 // Row direction is Y 

 unsigned int src_ri = other.index(pos.x + src_min.x, pos.y + src_min.y, pos.z + src_min.z); 

 unsigned int dst_ri = index(pos.x + dst_min.x, pos.y + dst_min.y, pos.z + dst_min.z); 

 memcpy(&channel.data[dst_ri], &other_channel.data[src_ri], area_size.y * sizeof(uint8_t)); 

 } 

 } 

 } else if (channel.defval != other_channel.defval) { 

 if (channel.data == NULL) { 

 create_channel(channel_index, _size, channel.defval); 

 } 

 // Set row by row 

 Vector3i pos; 

 for (pos.z = 0; pos.z < area_size.z; ++pos.z) { 

 for (pos.x = 0; pos.x < area_size.x; ++pos.x) { 

 unsigned int dst_ri = index(pos.x + dst_min.x, pos.y + dst_min.y, pos.z + dst_min.z); 

 memset(&channel.data[dst_ri], other_channel.defval, area_size.y * sizeof(uint8_t)); 

 } 

 } 

 } 

 } 

 }

This line:

godot_voxel/voxel_buffer.cpp

Line 229 in 7ced53b

if (area_size == _size) {

Should be if (other._size == _size) { because we can only the other VoxelBuffer::copy_from only if the other and current one size's are equals. Even if the area_size is equals to the destiny of the copy, we won't provide any offset, so it'll always fail.

Should I open another issue or since their are related, we can keep both here?

from godot_voxel.

Zylann commented on August 16, 2024

Actually copy_from without arguments can only be used if other._size == _size AND if the area specified covers the whole block anyways.

from godot_voxel.

afonsolage commented on August 16, 2024

But what if I want to copy an area that is the same size of target buffer? It will evaluate to true and will crash on another copy_from, since there is the following check: ERR_FAIL_COND(other._size == _size);

Let me give an use case. Let's say I have a padded voxel buffer of size (18, 18, 18). What if I want to copy only the "original-nonpadded-area" of the buffer into another buffer of size(16, 16, 16)?

I can just call the function copy_from as follows:

shoter_buffer->copy_from(**padded_buffer, Vector3i(buffer_padding), padded_buffer->get_size() - buffer_padding, Vector3i(), channel);

But it'll crash, because since the area_size == _size it'll try to make a direct copy using the other copy_from. But if we change the check to other._size == _size it'll still do the area copy and will work fine.

from godot_voxel.

Zylann commented on August 16, 2024

Correcting your example, should be 2*padding:

padded_buffer->get_size() - 2 * buffer_padding

Otherwise yeah sorry, what I meant is that if the two buffers have the same size AND the desired area is also that same size, then the copy_from without area could be used. In any other case, it should continue with the area method.

from godot_voxel.

afonsolage commented on August 16, 2024

So the correct way should be:

 if (area_size == _size && _other.size == _size) {

Is that so?

Also, thanks for pointing me out that error, you fixed a future bug on my voxel lighting code 😄

from godot_voxel.

Zylann commented on August 16, 2024

Yes that check would be correct.
Good to hear :D

from godot_voxel.

Zylann commented on August 16, 2024

Fixed in 3074f82

from godot_voxel.

Understanding VoxelBuffer::copy_from fail condition about godot_voxel HOT 8 CLOSED

Comments (8)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent

	void VoxelBuffer::copy_from(const VoxelBuffer &other, Vector3i src_min, Vector3i src_max, Vector3i dst_min, unsigned int channel_index) {

	ERR_FAIL_INDEX(channel_index, MAX_CHANNELS);

	Channel &channel = _channels[channel_index];
	const Channel &other_channel = other._channels[channel_index];

	Vector3i::sort_min_max(src_min, src_max);

	src_min.clamp_to(Vector3i(0, 0, 0), other._size);
	src_max.clamp_to(Vector3i(0, 0, 0), other._size + Vector3i(1, 1, 1));

	dst_min.clamp_to(Vector3i(0, 0, 0), _size);
	Vector3i area_size = src_max - src_min;
	//Vector3i dst_max = dst_min + area_size;

	if (area_size == _size) {
	copy_from(other, channel_index);
	} else {
	if (other_channel.data) {
	if (channel.data == NULL) {
	create_channel(channel_index, _size, channel.defval);
	}
	// Copy row by row
	Vector3i pos;
	for (pos.z = 0; pos.z < area_size.z; ++pos.z) {
	for (pos.x = 0; pos.x < area_size.x; ++pos.x) {
	// Row direction is Y
	unsigned int src_ri = other.index(pos.x + src_min.x, pos.y + src_min.y, pos.z + src_min.z);
	unsigned int dst_ri = index(pos.x + dst_min.x, pos.y + dst_min.y, pos.z + dst_min.z);
	memcpy(&channel.data[dst_ri], &other_channel.data[src_ri], area_size.y * sizeof(uint8_t));
	}
	}
	} else if (channel.defval != other_channel.defval) {
	if (channel.data == NULL) {
	create_channel(channel_index, _size, channel.defval);
	}
	// Set row by row
	Vector3i pos;
	for (pos.z = 0; pos.z < area_size.z; ++pos.z) {
	for (pos.x = 0; pos.x < area_size.x; ++pos.x) {
	unsigned int dst_ri = index(pos.x + dst_min.x, pos.y + dst_min.y, pos.z + dst_min.z);
	memset(&channel.data[dst_ri], other_channel.defval, area_size.y * sizeof(uint8_t));
	}
	}
	}
	}
	}