Fix #104370: Draw: Don't request the same attribute more than once #104709

Miguel Pozo · 2023-02-13T18:26:43+01:00

Miguel Pozo commented

2023-02-13 18:26:43 +01:00

Avoid running out of attributes when multiple material slots use the same one.

Cleanup:
Removes the return value from drw_attributes_add_request since it shouldn't be modified afterwards and it's never used.
Avoid making copies of DRW_AttributeRequest in drw_attributes_has_request.

Avoid running out of attributes when multiple material slots use the same one. Cleanup: Removes the return value from drw_attributes_add_request since it shouldn't be modified afterwards and it's never used. Avoid making copies of DRW_AttributeRequest in drw_attributes_has_request.

Miguel Pozo added this to the EEVEE & Viewport project 2023-02-13 18:26:53 +01:00

Miguel Pozo requested review from Hans Goudey 2023-02-13 18:27:11 +01:00

Miguel Pozo requested review from Clément Foucault 2023-02-13 18:27:26 +01:00

Miguel Pozo changed title from ~~Fix 104370: Draw: Don't request the same attribute more than once~~ to Fix #104370: Draw: Don't request the same attribute more than once

2023-02-13 18:27:57 +01:00

Hans Goudey requested changes 2023-02-13 18:34:37 +01:00

Hans Goudey left a comment

I'm a bit surprised this is necessary, since it looks like drw_attributes_merge is meant to handle this already. I don't really know why, but it looks like a function is meant to add requests to a local DRW_Attributes, then merge it with the cache's final requests.

Might be nice to just simplify that with a threadsafe (write-protected with a lock) add in the first place?

Eventually it would be much nicer to store the requests with a VectorSet IMO.

I'm a bit surprised this is necessary, since it looks like `drw_attributes_merge` is meant to handle this already. I don't really know why, but it looks like a function is meant to add requests to a local `DRW_Attributes`, then merge it with the cache's final requests. Might be nice to just simplify that with a threadsafe (write-protected with a lock) add in the first place? Eventually it would be much nicer to store the requests with a `VectorSet` IMO.

source/blender/draw/intern/draw_attributes.cc

						
				@ -71,6 +71,14 @@ DRW_AttributeRequest *drw_attributes_add_request(DRW_Attributes *attrs,

				    return nullptr;

				  }

				  for (int i : blender::IndexRange(attrs->num_requests)) {

Hans Goudey commented

2023-02-13 18:32:11 +01:00

Hmm, seems like this logic should be deduplicated with drw_attributes_has_request, which is basically the same except it returns a boolean.

Hmm, seems like this logic should be deduplicated with `drw_attributes_has_request`, which is basically the same except it returns a boolean.

Miguel Pozo commented

2023-02-13 19:35:31 +01:00

The DRW_Attributes parameter in drw_attributes_has_request is const, but drw_attributes_add_request returns a non-const one.

There's not a single call to drw_attributes_add_request that actually uses the return value, though, so we could simply remove it or return a const result.

The `DRW_Attributes` parameter in `drw_attributes_has_request` is const, but `drw_attributes_add_request` returns a non-const one. There's not a single call to `drw_attributes_add_request` that actually uses the return value, though, so we could simply remove it or return a const result.

pragma37 marked this conversation as resolved

Miguel Pozo commented

2023-02-13 19:15:49 +01:00

I'm a bit surprised this is necessary, since it looks like drw_attributes_merge is meant to handle this already.

The issue is that this is at the beginning of the function:

if (attrs->num_requests >= GPU_MAX_ATTR) {
	return nullptr;
}

I guess it would be possible to call drw_attributes_merge for every loop iteration in mesh_cd_calc_used_gpu_layers and pointcloud_surface_shaded_get, but that seems more fragile to me, and likely slower.

Eventually it would be much nicer to store the requests with a VectorSet IMO.

Does it worth the overhead?
In any case, I'm not sure what's the rule about converting headers to C++.
Is it ok as long as it compiles, or there's more to it?

> I'm a bit surprised this is necessary, since it looks like `drw_attributes_merge` is meant to handle this already. The issue is that this is at the beginning of the function: ``` if (attrs->num_requests >= GPU_MAX_ATTR) { return nullptr; } ``` I guess it would be possible to call `drw_attributes_merge` for every loop iteration in `mesh_cd_calc_used_gpu_layers` and `pointcloud_surface_shaded_get`, but that seems more fragile to me, and likely slower. > Eventually it would be much nicer to store the requests with a VectorSet IMO. Does it worth the overhead? In any case, I'm not sure what's the rule about converting headers to C++. Is it ok as long as it compiles, or there's more to it?

Miguel Pozo force-pushed pull-fix-104370 from 0c7ce9d188 to 71dece7bde

2023-02-15 20:00:28 +01:00

Compare

Miguel Pozo commented

2023-02-15 20:12:45 +01:00

I've been looking into better ways to solve this as we talked about on chat, but that would require a big refactor.
While the VBOs are indeed only binded to the relevant batches, all the loading logic assumes a single batch.

I've fixed the code duplication and removed the return value since it didn't make too much sense (it shouldn't be modified afterwards) and no caller used it.
I've also updated drw_attributes_has_request to avoid making copies of DRW_AttributeRequest.

Other than that, I'd merge the fix as it is now and leave possible improvements for the future.

[I've been looking](https://projects.blender.org/pragma37/blender/commit/6d830e68893699f766c322277f22899477ff5ea7) into better ways to solve this as we talked about on chat, but that would require a big refactor. While the VBOs are indeed only binded to the relevant batches, all the loading logic assumes a single batch. I've fixed the code duplication and removed the return value since it didn't make too much sense (it shouldn't be modified afterwards) and no caller used it. I've also updated drw_attributes_has_request to avoid making copies of DRW_AttributeRequest. Other than that, I'd merge the fix as it is now and leave possible improvements for the future.

pragma37 changed target branch from main to blender-v3.5-release

2023-02-15 20:15:33 +01:00

Hans Goudey approved these changes 2023-02-15 21:03:00 +01:00

Hans Goudey left a comment

Looks good now. Agreed that further changes should happen separately from this.
IMO some of the info from your latest comment should end up in the commit message.

Looks good now. Agreed that further changes should happen separately from this. IMO some of the info from your latest comment should end up in the commit message.

Miguel Pozo merged commit 7a76f2ae77 into blender-v3.5-release

2023-02-15 23:55:02 +01:00

Miguel Pozo deleted branch pull-fix-104370

2023-02-15 23:55:04 +01:00

Miguel Pozo referenced this issue from a commit

2023-02-15 23:55:04 +01:00

Fix #104370: Draw: Don't request the same attribute more than once

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #104370: Draw: Don't request the same attribute more than once #104709