Geometry Nodes: Visualize viewer as text #115664

Merged
Jacques Lucke merged 28 commits from Julian-Plak/blender:viewer-node-values into main 2023-12-19 13:30:28 +01:00
Contributor

Geometry Nodes: Visualize the attributes of the viewer node in the viewport.

image


As discussed: WIP PR for visualising GN values.

To-do:

  • Draw attribute text from viewer on vertex
    - improve rendering performance by limiting text overlap
    - - improve overlapping algorithm (?)
  • geometry types
    • meshes
    • instances
    • curves
    • points
  • Add a way to toggle text overlay in the UI
  • Visualize all attribute types (e.g. float2, float3)
  • Visualize on all domains
    • points
    • edges
    • faces
    • face corners
    • splines
    • instances

future plans:

  • center text on vertex
  • readable text (discussed in blender.chat)
Geometry Nodes: Visualize the attributes of the viewer node in the viewport. ![image](/attachments/98b95fc1-0327-40b6-8532-36e4b056e6d4) --- As [discussed](https://devtalk.blender.org/t/visualize-attributes-geometry-nodes/32312/6): WIP PR for visualising GN values. **To-do:** - [x] Draw attribute text from viewer on vertex ~~- [ ] improve rendering performance by limiting text overlap~~ ~~- - [ ] improve overlapping algorithm (?)~~ - [x] geometry types - - [x] meshes - - [x] instances - - [x] curves - - [x] points - [x] Add a way to toggle text overlay in the UI - [x] Visualize all attribute types (e.g. float2, float3) - [x] Visualize on all domains - [x] points - [x] edges - [x] faces - [x] face corners - [x] splines - [x] instances **future plans:** - center text on vertex - readable text (discussed in blender.chat)
149 KiB
Julian-Plak added 5 commits 2023-12-01 13:15:10 +01:00
Iliya Katushenock added this to the EEVEE & Viewport project 2023-12-01 13:25:51 +01:00
Iliya Katushenock added the
Module
Modeling
label 2023-12-01 13:25:58 +01:00
Falk David reviewed 2023-12-01 13:58:41 +01:00
Falk David left a comment
Member

Some general feedback:

  • Use the vec types e.g. float2 instead of float[2]s. Functions that need a float[2] or float * will accept float2 just fine. This makes math a lot easier to write. Functions like mul_m4_v3 shouldn't be used in new code (use math::transform_point).
  • In new code, we tend to not use manual memory allocations (e.g. MEM_mallocN). Use our data-structurs from BLI instead. So the grid can just be an Array<Array<int>> (Maybe it should be just a flat array though). You shouldn't need to worry about allocation and freeing.
  • There is no need for a point struct here. pos can be accessed in the second loop position_attributes[i]. cell can be an Array<int2>, val can be an Array<float>, and valid can be an Array<bool>. We generally use an SoA rather than an AoS approach.
Some general feedback: * Use the vec types e.g. `float2` instead of `float[2]`s. Functions that need a `float[2]` or `float *` will accept `float2` just fine. This makes math a lot easier to write. Functions like `mul_m4_v3` shouldn't be used in new code (use `math::transform_point`). * In new code, we tend to not use manual memory allocations (e.g. `MEM_mallocN`). Use our data-structurs from `BLI` instead. So the `grid` can just be an `Array<Array<int>>` (Maybe it should be just a flat array though). You shouldn't need to worry about allocation and freeing. * There is no need for a `point` struct here. `pos` can be accessed in the second loop `position_attributes[i]`. `cell` can be an `Array<int2>`, `val` can be an `Array<float>`, and `valid` can be an `Array<bool>`. We generally use an SoA rather than an AoS approach.
Falk David reviewed 2023-12-01 14:17:19 +01:00
Falk David left a comment
Member

Something else I noticed: In the second loop, you use

if (!p.valid) {
   continue;
}

to skip over invalid points. This is fine, but we can leverage some nice blender utilities here. What you basically want is an IndexMask and then iterate over all the indices that are in the mask.
If you have the Array<bool> valid from my comment above, you can simply do:

IndexMaskMemory memory;
const IndexMask valid_points = IndexMask::from_bools(valid);

and then iterate over the indices in the mask directly

valid_points.foreach_index([&](const int64_t i) {
   ...
});
Something else I noticed: In the second loop, you use ``` if (!p.valid) { continue; } ``` to skip over invalid points. This is fine, but we can leverage some nice blender utilities here. What you basically want is an `IndexMask` and then iterate over all the indices that are in the mask. If you have the `Array<bool> valid` from my comment above, you can simply do: ``` IndexMaskMemory memory; const IndexMask valid_points = IndexMask::from_bools(valid); ``` and then iterate over the indices in the mask directly ``` valid_points.foreach_index([&](const int64_t i) { ... }); ```
Hans Goudey added the
Interest
Geometry Nodes
Interest
Nodes & Physics
labels 2023-12-01 16:00:55 +01:00
Member

Thanks for working on this! For landing the change, I think a better approach would be to split the grid-based hiding into a separate commit. This first PR should focus on just displaying the data in the viewport and adding the option to the UI. Then we can look at the other features after that.

Thanks for working on this! For landing the change, I think a better approach would be to split the grid-based hiding into a separate commit. This first PR should focus on just displaying the data in the viewport and adding the option to the UI. Then we can look at the other features after that.
Author
Contributor

Thanks for working on this! For landing the change, I think a better approach would be to split the grid-based hiding into a separate commit. This first PR should focus on just displaying the data in the viewport and adding the option to the UI. Then we can look at the other features after that.

I understand, but limiting the amount of drawn verts is a performance measure, rather than a UX feature! Rendering text with high density has a lot of overdraw, bricking my system very fast. This grid system fixes that and has the added benefit of improving readability. Would you have alternative suggestions to improve frame times with high text density?

edit: Or if it's okay, I could finish this PR first and do the performance improvements in the next PR!

> Thanks for working on this! For landing the change, I think a better approach would be to split the grid-based hiding into a separate commit. This first PR should focus on just displaying the data in the viewport and adding the option to the UI. Then we can look at the other features after that. I understand, but limiting the amount of drawn verts is a performance measure, rather than a UX feature! Rendering text with high density has a lot of overdraw, bricking my system very fast. This grid system fixes that and has the added benefit of improving readability. Would you have alternative suggestions to improve frame times with high text density? edit: Or if it's okay, I could finish this PR first and do the performance improvements in the next PR!
Member

Just getting the text on the screen is a great start, and there's enough going on with that already. Text has been drawn in the same way for the edit mode indices overlay, so these aren't exactly new problems.

Just getting the text on the screen is a great start, and there's enough going on with that already. Text has been drawn in the same way for the edit mode indices overlay, so these aren't exactly new problems.
Julian-Plak force-pushed viewer-node-values from 774c69a8da to 1386cc28dc 2023-12-03 22:23:37 +01:00 Compare
Hans Goudey requested changes 2023-12-03 22:30:17 +01:00
@ -220,0 +240,4 @@
UI_GetThemeColor4ubv(TH_DRAWEXTRA_FACEANG, col);
Mesh *me = static_cast<Mesh *>(object.data);
Member

A const reference is a bit nicer here :) Also how about mesh instead of me?

A const reference is a bit nicer here :) Also how about `mesh` instead of `me`?
Julian-Plak marked this conversation as resolved
@ -220,0 +243,4 @@
Mesh *me = static_cast<Mesh *>(object.data);
const AttributeAccessor attributes = me->attributes();
const VArray viewer_attributes = *attributes.lookup<float3>(".viewer");
Member

Read the attributes as a float since you only display one value per element anyway.

Read the attributes as a `float` since you only display one value per element anyway.
Member

Though this is not a concern for this PR, I think we should allow float, float2, float3, etc. and render them as e.g. 0.0, (0.0, 0.0), (0.0, 0.0, 0.0) etc. in the future.

Though this is not a concern for this PR, I think we should allow `float`, `float2`, `float3`, etc. and render them as e.g. `0.0`, `(0.0, 0.0)`, `(0.0, 0.0, 0.0)` etc. in the future.
Author
Contributor

I'll focus on the float for this PR, and get started on the array notation when this PR is merged!

I'll focus on the float for this PR, and get started on the array notation when this PR is merged!
Julian-Plak marked this conversation as resolved
@ -220,0 +244,4 @@
const AttributeAccessor attributes = me->attributes();
const VArray viewer_attributes = *attributes.lookup<float3>(".viewer");
const VArray position_attributes = *attributes.lookup<float3>("position");
Member

const Span<float3> positions = mesh.positions();

`const Span<float3> positions = mesh.positions();`
Julian-Plak marked this conversation as resolved
@ -220,0 +248,4 @@
float4x4 object_to_world = float4x4(object.object_to_world);
for (int i = 0; i < me->totvert; i++) {
Member

for (const int i : positions.index_range()) {

`for (const int i : positions.index_range()) {`
Julian-Plak marked this conversation as resolved
@ -575,3 +574,1 @@
V3D_OVERLAY_SCULPT_SHOW_FACE_SETS = (1 << 15),
V3D_OVERLAY_SCULPT_CURVES_CAGE = (1 << 16),
V3D_OVERLAY_SHOW_LIGHT_COLORS = (1 << 17),
V3D_OVERLAY_VIEWER_ATTRIBUTE_TEXT = (1 << 14),
Member

Existing values shouldn't be changed, since those are stored in files. It's fine to just add a new flag at the end.

Existing values shouldn't be changed, since those are stored in files. It's fine to just add a new flag at the end.
Julian-Plak marked this conversation as resolved
@ -4562,0 +4563,4 @@
RNA_def_property_boolean_sdna(prop, nullptr, "overlay.flag", V3D_OVERLAY_VIEWER_ATTRIBUTE_TEXT);
RNA_def_property_clear_flag(prop, PROP_ANIMATABLE);
RNA_def_property_ui_text(
prop, "View attribute text", "Show attribute values as text on vertices");
Member

Capitalize UI labels: View Attribute Text

Capitalize UI labels: `View Attribute Text`
Julian-Plak marked this conversation as resolved
Julian-Plak added 1 commit 2023-12-03 22:44:55 +01:00
Julian-Plak added 1 commit 2023-12-04 13:33:24 +01:00
Falk David reviewed 2023-12-04 14:09:01 +01:00
@ -220,0 +279,4 @@
}
}
}
return std::make_tuple(viewer_attributes, positions);
Member

Might make sense to make this a struct since it reads a bit nicer in the other parts and would allow us to easily add more fields when we need other info. Something like

struct ViewerData {
   Span<float3> positions;
   VArray<float> attribute;
};

@HooglyBoogly Do you agree?

Might make sense to make this a `struct` since it reads a bit nicer in the other parts and would allow us to easily add more fields when we need other info. Something like ``` struct ViewerData { Span<float3> positions; VArray<float> attribute; }; ``` @HooglyBoogly Do you agree?
Member

I'd just pass these around separately. There's no need to put them in one struct.

I'd just pass these around separately. There's no need to put them in one struct.
filedescriptor marked this conversation as resolved
Julian-Plak added 1 commit 2023-12-04 14:10:37 +01:00
Author
Contributor

I've had a look at fitting the UI and I'd like to propose 2 options.

The old overlay UI fits "Viewer Node" as a slider under the "Geometry Category". The first option would be to include a "Viewer Node Text" checkbox underneath this slider.
image

The second option takes into account that there's now a category of "viewer node" options in the UI, by splitting both the opacity slider, as well as the text box into a new "Viewer Node" category.
image

This also allows to give the slider a more descriptive name, like "overlay opacity", "overlay color" or something like that, as it's part of the Viewer Node category.

Would love to hear what you guys think! Latest option was implemented in the last commit.

I've had a look at fitting the UI and I'd like to propose 2 options. The old overlay UI fits "Viewer Node" as a slider under the "Geometry Category". The first option would be to include a "Viewer Node Text" checkbox underneath this slider. ![image](/attachments/61917dba-9895-4320-b141-1725252dc3e5) The second option takes into account that there's now a category of "viewer node" options in the UI, by splitting both the opacity slider, as well as the text box into a new "Viewer Node" category. ![image](/attachments/5eea6dc2-2466-4bf8-ba4f-064aa543b57e) This also allows to give the slider a more descriptive name, like "overlay opacity", "overlay color" or something like that, as it's part of the Viewer Node category. Would love to hear what you guys think! Latest option was implemented in the last commit.
Member

The second mockup looks good to me! Unfortunately it makes the popover a bit taller, but there isn't really any way around that, and I bet we'll need more settings here in the future too.

The second mockup looks good to me! Unfortunately it makes the popover a bit taller, but there isn't really any way around that, and I bet we'll need more settings here in the future too.
Julian-Plak added 1 commit 2023-12-04 14:45:28 +01:00
Julian-Plak added 1 commit 2023-12-04 14:53:08 +01:00
Author
Contributor

@filedescriptor @HooglyBoogly
I think I'm about ready to remove WIP status from this PR. Would you guys also say the PR is feature complete like this?

@filedescriptor @HooglyBoogly I think I'm about ready to remove WIP status from this PR. Would you guys also say the PR is feature complete like this?
Julian-Plak added 1 commit 2023-12-04 15:00:36 +01:00
Hans Goudey reviewed 2023-12-04 15:01:26 +01:00
Hans Goudey left a comment
Member

Functionality wise, this looks reasonable, yeah. I do wonder if people will expect vector attributes to work, but to me it's fine to keep this minimal to start.

Functionality wise, this looks reasonable, yeah. I do wonder if people will expect vector attributes to work, but to me it's fine to keep this minimal to start.
@ -220,0 +242,4 @@
VArray<float> viewer_attributes;
Span<float3> positions;
switch (type) {
Member

I'd suggest structuring this a bit differently. There could be one function that does the DRW_text_cache_add given a span of positions and the attribute. Then at the top level there can be a switch for the object types that calls this. That way this temporary data becomes unnecessary.

I'd suggest structuring this a bit differently. There could be one function that does the `DRW_text_cache_add` given a span of positions and the attribute. Then at the top level there can be a switch for the object types that calls this. That way this temporary data becomes unnecessary.
Author
Contributor

Done! I took reference from the Overlay engine functions and created a seperate function here: 135089a07e and here: cfcad866c8

How's this?

Done! I took reference from the Overlay engine functions and created a seperate function here: https://projects.blender.org/blender/blender/commit/135089a07eff8f33520cea543ffa0bb188b7d669 and here: https://projects.blender.org/blender/blender/commit/cfcad866c83239d924a44f8413a6bd4d37281378 How's this?
Julian-Plak marked this conversation as resolved
Falk David requested changes 2023-12-04 15:01:53 +01:00
Falk David left a comment
Member

Sure. I have some cleanup comments.

Sure. I have some cleanup comments.
@ -404,6 +406,12 @@ static void OVERLAY_cache_populate(void *vedata, Object *ob)
}
}
if (pd->overlay.flag & V3D_OVERLAY_VIEWER_ATTRIBUTE_TEXT) {
Member

if (is_preview && (pd->overlay.flag & V3D_OVERLAY_VIEWER_ATTRIBUTE_TEXT != 0)) {

`if (is_preview && (pd->overlay.flag & V3D_OVERLAY_VIEWER_ATTRIBUTE_TEXT != 0)) {`
Julian-Plak marked this conversation as resolved
@ -9,0 +9,4 @@
#include "DNA_curve_types.h"
#include "DNA_pointcloud_types.h"
#define DEBUG_TIME
Member

This can probably be removed now.

This can probably be removed now.
Julian-Plak marked this conversation as resolved
Julian-Plak added 1 commit 2023-12-04 16:15:14 +01:00
Julian-Plak added 1 commit 2023-12-04 16:19:14 +01:00
Julian-Plak added 1 commit 2023-12-04 16:25:04 +01:00
Julian-Plak changed title from WIP: Visualize attributes - Geometry Nodes to WIP: Geometry Nodes: Visualize viewer as text 2023-12-04 16:36:31 +01:00
Falk David reviewed 2023-12-04 16:40:16 +01:00
@ -8,6 +8,9 @@
#pragma once
#include "BLI_math_vector_types.hh"
Member

I guess these should be in an #ifdef __cplusplus ?

I guess these should be in an `#ifdef __cplusplus` ?
Author
Contributor

Stupid question, but should the function that references it also be wrapped in an #ifdef __cplusplus?

void DRW_text_viewer_attribute(blender::VArray<float> attributes,
                               blender::Span<blender::float3> positions,
                               float4x4 modelMatrix);
Stupid question, but should the function that references it also be wrapped in an `#ifdef __cplusplus`? ```C void DRW_text_viewer_attribute(blender::VArray<float> attributes, blender::Span<blender::float3> positions, float4x4 modelMatrix); ```
Member

draw_manager_text.h seems to be included from .cc files only, so this is why it works. But IMHO the includes and the function declaration should be wrapped in an

#ifdef __cplusplus
...
#endif

(outside the extern "C" {).

`draw_manager_text.h` seems to be included from `.cc` files only, so this is why it works. But IMHO the includes and the function declaration should be wrapped in an ``` #ifdef __cplusplus ... #endif ``` (outside the `extern "C" {`).
Julian-Plak marked this conversation as resolved
Member

I think it's very reasonable to expect that this supports all viewable attribute types. Feels like that shouldn't be too hard if you can already view one kind of attribute.

Also, currently the patch seems to assume that the .viewer attribute is on the point domain.

I think it's very reasonable to expect that this supports all viewable attribute types. Feels like that shouldn't be too hard if you can already view one kind of attribute. Also, currently the patch seems to assume that the `.viewer` attribute is on the point domain.
Julian-Plak added 1 commit 2023-12-05 15:10:20 +01:00
Author
Contributor

I think it's very reasonable to expect that this supports all viewable attribute types. Feels like that shouldn't be too hard if you can already view one kind of attribute.

Also, currently the patch seems to assume that the .viewer attribute is on the point domain.

Checking multiple viewer attribute types right now!

Is visualising attributes on different domains also something you'd like to see in this PR? representation of values still works technically when switching domains, but may not be accurate.
image (center and bottom vert displayed as 0 while value attribute = 2)

> I think it's very reasonable to expect that this supports all viewable attribute types. Feels like that shouldn't be too hard if you can already view one kind of attribute. > > Also, currently the patch seems to assume that the `.viewer` attribute is on the point domain. Checking multiple viewer attribute types right now! Is visualising attributes on different domains also something you'd like to see in this PR? representation of values still works technically when switching domains, but may not be accurate. ![image](/attachments/f6678158-bfe5-4a3d-b360-8fbf95881c4d) (center and bottom vert displayed as 0 while value attribute = 2)
122 KiB
Member

Is visualising attributes on different domains also something you'd like to see in this PR?

Yes, think that's a core part of this patch. It should be fairly straight forward. You can just retrieve the position attribute on the same domain that the .viewer attribute is on. The interpolation should happen automatically.

> Is visualising attributes on different domains also something you'd like to see in this PR? Yes, think that's a core part of this patch. It should be fairly straight forward. You can just retrieve the `position` attribute on the same domain that the `.viewer` attribute is on. The interpolation should happen automatically.
Julian-Plak added 1 commit 2023-12-05 16:04:37 +01:00
Author
Contributor

Is visualising attributes on different domains also something you'd like to see in this PR?

Yes, think that's a core part of this patch. It should be fairly straight forward. You can just retrieve the position attribute on the same domain that the .viewer attribute is on. The interpolation should happen automatically.

Works great!

image
image
image

It does look like the instance domain requires a different approach. Looking at it now.

> > Is visualising attributes on different domains also something you'd like to see in this PR? > > Yes, think that's a core part of this patch. It should be fairly straight forward. You can just retrieve the `position` attribute on the same domain that the `.viewer` attribute is on. The interpolation should happen automatically. Works great! ![image](/attachments/80e29028-f9ed-4638-9a6a-10b806ba48b8) ![image](/attachments/c30b23b0-5fa2-4328-abc8-1e6ab6c3b28d) ![image](/attachments/a8a2b93c-fe7e-417e-94d4-958d3b1a3fb2) It does look like the instance domain requires a different approach. Looking at it now.
Julian-Plak added 1 commit 2023-12-12 13:00:23 +01:00
Julian-Plak force-pushed viewer-node-values from 3352cc79aa to 4d8a102be5 2023-12-12 13:13:19 +01:00 Compare
Julian-Plak force-pushed viewer-node-values from 4d8a102be5 to 469de16244 2023-12-12 13:14:03 +01:00 Compare
Julian-Plak added 1 commit 2023-12-12 19:31:27 +01:00
Julian-Plak requested review from Hans Goudey 2023-12-12 19:36:22 +01:00
Julian-Plak requested review from Falk David 2023-12-12 19:36:24 +01:00
Author
Contributor

@HooglyBoogly @filedescriptor @JacquesLucke

Hi guys! I think this PR is feature complete. Please let me know your thoughts!

@HooglyBoogly @filedescriptor @JacquesLucke Hi guys! I think this PR is feature complete. Please let me know your thoughts!
Hans Goudey changed title from WIP: Geometry Nodes: Visualize viewer as text to Geometry Nodes: Visualize viewer as text 2023-12-12 19:38:27 +01:00
Julian-Plak requested review from Jacques Lucke 2023-12-12 22:19:31 +01:00
Hans Goudey requested changes 2023-12-13 02:05:49 +01:00
@ -7029,0 +7040,4 @@
row.prop(overlay, "show_viewer_attribute", text="")
subrow = row.row(align=True)
subrow.active = overlay.show_viewer_attribute
subrow.prop(overlay, "viewer_attribute_opacity", text="Overlay Color")
Member

This whole popover contains overlays, so this can be "Color Opacity" That should be a bit more descriptive

This whole popover contains overlays, so this can be "Color Opacity" That should be a bit more descriptive
Julian-Plak marked this conversation as resolved
@ -0,0 +1,141 @@
#include "BKE_attribute.hh"
Member

Missing license text and copyright, you can copy it from other files

Missing license text and copyright, you can copy it from other files
Julian-Plak marked this conversation as resolved
@ -0,0 +17,4 @@
#include <optional>
using namespace blender;
using namespace blender::bke;
Member

Generally we avoid this sort of using namespace at the top of files. Either the file should be in the blender::draw namespace, or these should be at the top of every function. Best to skip using namespace blender::bke; completely though, since the bke:: prefix can be helpful to the reader. It's the replacement for the BKE_ prefix from the C functions.

Generally we avoid this sort of `using namespace` at the top of files. Either the file should be in the `blender::draw` namespace, or these should be at the top of every function. Best to skip `using namespace blender::bke;` completely though, since the `bke::` prefix can be helpful to the reader. It's the replacement for the `BKE_` prefix from the C functions.
Julian-Plak marked this conversation as resolved
@ -0,0 +18,4 @@
using namespace blender;
using namespace blender::bke;
Member

Try to structure this so that we don't switch based on the type for every index. You can use a templated function that processes the entire array at once. Putting this sort of type switching inside a hot loop makes things hard to optimize.

For instances, the span of positions and values you pass can just have size 1

Try to structure this so that we don't switch based on the type for every index. You can use a templated function that processes the entire array at once. Putting this sort of type switching inside a hot loop makes things hard to optimize. For instances, the span of positions and values you pass can just have size 1
Julian-Plak marked this conversation as resolved
@ -0,0 +19,4 @@
using namespace blender;
using namespace blender::bke;
static void add_data_based_on_type(GVArray attributes,
Member

GVArray attributes -> const GVArray &attributes

`GVArray attributes` -> `const GVArray &attributes`
Julian-Plak marked this conversation as resolved
@ -0,0 +40,4 @@
DRW_text_viewer_attribute(attributes.get<float3>(index), pos);
}
if (CD_PROP_COLOR == type || CD_PROP_QUATERNION == type) {
DRW_text_viewer_attribute(attributes.get<float4>(index), pos);
Member

This will probably fail for quaternions in debug builds, .get<float4>() will assert because the type isn't correct.

This will probably fail for quaternions in debug builds, `.get<float4>()` will assert because the type isn't correct.
Author
Contributor

I've been looking around the codebase to see alternative ways to cast quaternions, but I couldn't find it. Do you know what type I can retrieve quats as?

I've been looking around the codebase to see alternative ways to cast quaternions, but I couldn't find it. Do you know what type I can retrieve quats as?
Member

math::Quaternion should do it, then you can just explicitly cast it to float4 for display with float4(value).

`math::Quaternion` should do it, then you can just explicitly cast it to `float4` for display with `float4(value)`.
Julian-Plak marked this conversation as resolved
@ -0,0 +44,4 @@
}
}
// function for passing mesh, pointcloud, and curve data to text cache
Member

Check out the style guide: https://wiki.blender.org/wiki/Style_Guide/C_Cpp
Particularly the part that mentions comment style

Check out the style guide: https://wiki.blender.org/wiki/Style_Guide/C_Cpp Particularly the part that mentions comment style
Author
Contributor

Removed most comments as they seemed redundant. Thanks for the link!

Removed most comments as they seemed redundant. Thanks for the link!
Julian-Plak marked this conversation as resolved
@ -0,0 +48,4 @@
static void add_attributes_to_text_cache(AttributeAccessor attribute_accessor,
float4x4 modelMatrix)
{
if (attribute_accessor.contains(".viewer")) {
Member

Flip the condition and return early to have less indentation for the rest of the function

Flip the condition and return early to have less indentation for the rest of the function
Julian-Plak marked this conversation as resolved
@ -0,0 +79,4 @@
float3 pos = blender::math::transform_point(modelMatrix, loc);
eCustomDataType type = attribute_accessor.lookup_meta_data(".viewer")->data_type;
// and add that data to the text cache based on it's type
Member

it's -> its

`it's` -> `its`
Julian-Plak marked this conversation as resolved
@ -0,0 +131,4 @@
break;
}
case OB_CURVES: {
Curves *curves_id = static_cast<Curves *>(object.data);
Member

const Curves

Sane for the other object types

`const Curves` Sane for the other object types
Julian-Plak marked this conversation as resolved
@ -220,0 +228,4 @@
DRW_text_cache_add(dt, position, numstr, numstr_len, 0, 0, txt_flag, col);
}
void DRW_text_viewer_attribute(float attribute_value, blender::float3 position)
Member

I'm not sure putting these in a different file is helpful right now, feels a bit like a premature abstraction. Unless I'm missing something, couldn't this be implemented in overlay_viewer_text.cc directly?

I'm not sure putting these in a different file is helpful right now, feels a bit like a premature abstraction. Unless I'm missing something, couldn't this be implemented in `overlay_viewer_text.cc` directly?
Author
Contributor

I tried to keep it consistent, as this Text Manager file handles drawing indices in a similar way too. I can move the functions that format the data to overlay_viewer_text.cc!

I tried to keep it consistent, as this Text Manager file handles drawing indices in a similar way too. I can move the functions that format the data to `overlay_viewer_text.cc`!
Member

If you don't mind, that does sound simpler to me, and it means this PR would be adding less code too.

If you don't mind, that does sound simpler to me, and it means this PR would be adding less code too.
Julian-Plak marked this conversation as resolved
@ -53,2 +57,4 @@
}
#endif
#ifdef __cplusplus
Member

I moved this file to C++ to simplify this change (though based on another comment, a change in this file might not be necessary)

I moved this file to C++ to simplify this change (though based on another comment, a change in this file might not be necessary)
Julian-Plak marked this conversation as resolved
Julian-Plak added 1 commit 2023-12-14 11:24:34 +01:00
Julian-Plak force-pushed viewer-node-values from 97e77b6705 to f572ddc9e5 2023-12-16 12:09:06 +01:00 Compare
Julian-Plak added 1 commit 2023-12-16 12:18:01 +01:00
Julian-Plak added 1 commit 2023-12-16 12:20:26 +01:00
Julian-Plak requested review from Hans Goudey 2023-12-16 12:20:38 +01:00
Hans Goudey added 1 commit 2023-12-18 19:32:09 +01:00
b6e33298a9 Various cleanups and improvements to text format creation
- Handle integer attribute types
- Use "convert to static type" utility to handle all formats
- Fix handling of float colors (implicit conversion to float4 will give asserts)
- Handle byte colors
- Combine extraction to a single loop with `if constexpr` switches
- Standardize variable naming
- Avoid looking up theme color for every element
- Move code to C++ namespace
- Avoid a few unnecessary local variables
- Pass matrix by const reference
Hans Goudey approved these changes 2023-12-18 19:34:41 +01:00
Hans Goudey left a comment
Member

I updated the overlay_viewer_text.cc file, changes listed in the commit. Sorry to directly edit, I usually try to comment about changes here. But hopefully looking at the state of the code now will be helpful. Let me know if you have any questions.

I think this is in a good state to go in now. A solid foundation and clear next steps! I'll a day or so to commit the PR in case others want to review.

I updated the `overlay_viewer_text.cc` file, changes listed in the commit. Sorry to directly edit, I usually try to comment about changes here. But hopefully looking at the state of the code now will be helpful. Let me know if you have any questions. I think this is in a good state to go in now. A solid foundation and clear next steps! I'll a day or so to commit the PR in case others want to review.
Hans Goudey added 1 commit 2023-12-18 19:39:14 +01:00
Jacques Lucke approved these changes 2023-12-19 00:09:16 +01:00
@ -4565,0 +4566,4 @@
RNA_def_property_boolean_sdna(prop, nullptr, "overlay.flag", V3D_OVERLAY_VIEWER_ATTRIBUTE_TEXT);
RNA_def_property_clear_flag(prop, PROP_ANIMATABLE);
RNA_def_property_ui_text(
prop, "View Attribute Text", "Show attribute values as text on vertices");
Member

This description isn't accurate anymore.

This description isn't accurate anymore.
Julian-Plak marked this conversation as resolved
Author
Contributor

I updated the overlay_viewer_text.cc file, changes listed in the commit. Sorry to directly edit, I usually try to comment about changes here. But hopefully looking at the state of the code now will be helpful. Let me know if you have any questions.

I think this is in a good state to go in now. A solid foundation and clear next steps! I'll a day or so to commit the PR in case others want to review.

Man, perfect! thanks for helping out here. I'm learning a lot! Thanks for your patience 😄

> I updated the `overlay_viewer_text.cc` file, changes listed in the commit. Sorry to directly edit, I usually try to comment about changes here. But hopefully looking at the state of the code now will be helpful. Let me know if you have any questions. > > I think this is in a good state to go in now. A solid foundation and clear next steps! I'll a day or so to commit the PR in case others want to review. Man, perfect! thanks for helping out here. I'm learning a lot! Thanks for your patience 😄
Julian-Plak added 1 commit 2023-12-19 09:36:46 +01:00
Jacques Lucke merged commit b85011aee0 into main 2023-12-19 13:30:28 +01:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#115664
No description provided.