Overlay-Next: Armature #126474
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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 project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#126474
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "fclem/blender:overlay-next-armature"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Functional Changes:
Some shaders are duplicated and ported to the new
primitive expansion API.
The legacy code inside
overlay_armature.cc
have beenguarded behind
NO_LEGACY_OVERLAY
which canbe enabled to make sure no legacy code is used unnoticed.
This allows for spotting more easily code that needs to be
ported. Moreover, it is easier to remove this legacy code
when the time comes.
Rel #102179
Armatures
d8a0fe1e6cSelection is not working because of sub object flag. Have to investigate.
WIP: Overlay-Next: Armatureto Overlay-Next: ArmatureHow do I test this PR? Neither the PR description nor the PR it references describe that. Is it enough to enable the experimental feature? Does that immediately take effect, or do I need to restart Blender?
@dr.sybren It should take immediate effect, maybe you need to refresh the viewport.
However, if you want to test selection, you need to enable the experimental feature and restart blender. Otherwise it crashes.
Woah! Much work, and looks like improvement (and as said face-to-face: still much space for improving the old code ;-) )
There's quite a bit of
if (ctx->bone_buf) { …; return; }
. It's not immediately obvious whenctx->bone_buf
is going to be trueish/falsey, though. Is this an implicit detection that the new overlay system is in use? In that case I think it's better to add that as a comment to every such check, as it'll make it considerably easier to clean up (by someone who doesn't have an fclem-brain).Or, and this would have my preference, would be a
ctx->is_overlay_next()
function that just performs the same check in a more explicit way. For the new animation system we have something similar, seeAction::is_action_layered()
and friends.The big advantage of changing/documenting the code that way is that it makes it explicit that that thing being
nullptr
is invalid with overlay next. And thus it can be replaced by an assertion once the legacy code is removed.While playing around with a debug build, selecting a bone gives me:
@ -748,1 +712,3 @@
DRW_buffer_add_entry(ctx->envelope_distance, head_sph, tail_sph, xaxis);
if (ctx->bone_buf) {
ctx->bone_buf->envelope_distance_buf.append(
{*(float4 *)head_sph, *(float4 *)tail_sph, *(float3 *)xaxis},
This might need another of those
/* TODO(fclem): Cleanup these casts when Overlay Next is shipped. */
comments.@ -880,0 +905,4 @@
if (ctx->bone_buf && ctx->is_filled) {
ctx->bone_buf->custom_shape_fill
.lookup_or_add_cb(surf,
[ctx]() {
Oof, this is getting a bit messy. I think it'll be better to assign the lambda to a local variable and use that in the call.
Same for similar cases below.
@ -921,0 +984,4 @@
inst_data.set_color(float4(UNPACK3(color), wire_width / WIRE_WIDTH_COMPRESSION));
if (ctx->bone_buf) {
ctx->bone_buf->custom_shape_wire
This pattern is happening so often, I feel it would make sense to add a few methods for this on
bone_buf
, so that you can do something likectx->bone_buf->custom_shape_wire_append(ctx, geom, inst_data, select_id)
instead of repeating this block every time.@ -1016,0 +1148,4 @@
geom = ctx->shapes->arrows.get();
break;
case OB_EMPTY_IMAGE:
/* Not supported. */
Well, it draws an "empty image" just pefectly 😝
@ -1720,2 +1922,3 @@
/* Draw tip point */
if (select_id != -1) {
#ifndef NO_LEGACY_OVERLAY
if (select_id != -1 && ctx->bone_buf == nullptr) {
If my guess that the
ctx->bone_buf
check is to determine legacy/new status is right, I think it's better to always put that check first. That'll make it clearer which code path the remaining conditions are in.@ -1733,3 +1938,3 @@
}
else {
drw_shgroup_bone_point(ctx, bone.disp_tail_mat(), col_solid, col_hint_tail, col_wire_tail);
drw_shgroup_bone_sphere(ctx,
Thank you for getting rid of the
…_point
function that doesn't draw a point but a sphere 🧡🤍@ -2124,2 +2365,4 @@
const float(*disp_mat)[4] = bone.disp_mat();
if (ctx->bone_buf) {
/* TODO(fclem): This is the new pipeline. The code below it should then be removed. */
Check, so my suspicions are confirmed here. Probably better to code this in a way that makes this obvious.
@ -2193,2 +2481,4 @@
const float *col_hint = get_bone_hint_color(ctx, boneflag);
if (ctx->bone_buf) {
/* TODO(fclem): This is the new pipeline. The code below it should then be removed. */
Unclear what "then" refers to.
@ -2580,1 +2985,3 @@
for (eBone = static_cast<EditBone *>(arm->edbo->first), index = ob_orig->runtime->select_id;
for (eBone = static_cast<EditBone *>(arm->edbo->first),
/* Note: Selection Next handles the object id merging later. */
index = ctx->bone_buf ? 0x0 : ob_orig->runtime->select_id;
Yeah, here it's really unclear why
ctx->bone_buf
is checked, or what the relationship with the comment is. I have no idea what "object id merging" is, or why objects are relevant at all for selecting edit bones.Old code is mixing selection and drawing. It use to load the object
runtime->select_id
and increment by0x0001000
for each bone. So when drawing a bone its selection id is the combination of object and subobject id.New code expect that when drawing the bone, the selectino id is only equal to the subobject id (but still shifted). We do the merging of Object and SubObject selection ids in
SelectMap::select_id()
.If only we had the
select_id
as part ofEditBone::Runtime
for each bone that would be a bit more clear than this, but could potentially be slower. So I don't really want to change more than this.Maybe it is just a matter of making the comment clearer?
@ -2642,2 +3049,3 @@
ctx->draw_mode = ARM_DRAW_MODE_OBJECT; /* Will likely be set to ARM_DRAW_MODE_POSE below. */
ctx->draw_mode = ARM_DRAW_MODE_OBJECT; /* Will likely be set to
ARM_DRAW_MODE_POSE below. */
Rewrapping issue?
@ -2701,1 +3110,4 @@
/* TODO(fclem): Remove global access. This is only used for culling in selection mode.
* This is just a workaround for slow selection queries. Selection-next will not have this issue.
*/
const DRWView *view = is_pose_select ? DRW_view_default_get() : nullptr;
I assume that Selection Next is going to be done after Overlay Next?
It is done at the same time. It is one big project.
But removing / isolating the frustum culling code is more work than needed to make it work now.
@ -2776,2 +3188,3 @@
{
BLI_assert(BLI_memory_is_zero(ctx, sizeof(*ctx)));
/* Not true anymore. */
// BLI_assert(BLI_memory_is_zero(ctx, sizeof(*ctx)));
Then why not remove the assert altogether? Or replace it with a check that some things that are zero before this PR are still zero here? AFAIK this is just a check that the setup call isn't done twice, which can be done with any field that this function sets to a non-zero value.
I though it was to check that the struct is zero initialized and not default initialized (with undefined values).
The issue is that now the struct can be padded and have some bytes not initialized to zero (platform specific). But the default initialization does clear the expected fields.
But this is legacy code, so I wouldn't care too much about it.
@ -0,0 +110,4 @@
public:
Armatures(const SelectionType selection_type) : selection_type_(selection_type){};
void begin_sync(Resources &res, const State &state)
This function exists on many overlay structs. Wouldn't it be better to have some abstract superclass that defines this interface? That can then also hold the documentation for such functions. It'll also allow the use of the
override
keyword here, so that it's clear that this function is part of some API. Even when no code directly uses that superclass, the code-as-documentation benefits would be quite nice.If that's not an option, document this function. I have no idea whats purpose is, so I also can't really review it.
Well this is not an overlay thing but every piece of engine code inside of DRW module have the same structure.
That proposal about code as documentation is really interesting, but I think it is way out of scope of this PR. I added it as a follow up task to #102179.
@ -0,0 +145,4 @@
sub.shader_set(res.shaders.armature_envelope_fill.get());
sub.push_constant("alpha", 1.0f);
sub.push_constant("isDistance", true);
opaque.envelope_distance = ⊂
Use
this->
prefix to make it clear that this is not just some local variable, but modifyingthis
.I prefer using
_
suffix as they are private variables.@ -0,0 +183,4 @@
DRWState default_state = DRW_STATE_WRITE_COLOR | DRW_STATE_DEPTH_LESS_EQUAL |
DRW_STATE_WRITE_DEPTH | state.clipping_state;
{
For every anonymous block, add a little comment that makes it clear what the block is for. At least for the outer ones; the inner ones start with a line that's already quite indicative of what they're for.
@ -0,0 +393,4 @@
transparent.relations = opaque.relations;
}
auto clear_buffers = [](BoneBuffers &bb) {
Since this code only accesses properties of
bb
, wouldn't it make sense to have it as a method onBoneBuffers
?It's here as it is a design pattern that is used throughout the overlay next codebase. It increases locality of the code. And this is never supposed to be called anywhere else.
@ -0,0 +416,4 @@
bb.custom_shape_wire.clear();
};
clear_buffers(transparent);
After all the code above that seems to put stuff into
transparent
andopaque
, why are they cleared now? This could use a comment.@ -0,0 +433,4 @@
Resources *res = nullptr;
const ShapeCache *shapes = nullptr;
/* TODO: Legacy structures to be removed after overlay next is shipped. */
IMO it could make sense to prefix those fields with
legacy_
; that way their legacy status is clear at every use site.That makes the patch modify a lot more of legacy overlay code. Which I wanted to avoid. But I can batch rename them if that's wanted.
hmmmmm in that case better to keep it as-is for now. Or as a followup 'cleanup' commit/PR?
Added as follow up to #102179
@ -0,0 +539,4 @@
return;
}
auto end_sync = [&](BoneBuffers &bb) {
Same as in
begin_sync
, this seems to be "feature envy".Same answer then :) I suppose this is resolve now?
@ -37,2 +37,3 @@
{
MetaBall *mb = static_cast<MetaBall *>(ob_ref.object->data);
const Object *ob = ob_ref.object;
const MetaBall *mb = static_cast<MetaBall *>(ob->data);
Not sure how these changes are relevant to the Armature overlay?
I modified the constructor used for
circle_buf_.append
. But if you are talking about these two lines, then it is just small cleanup that I spotted doing the change.@ -118,0 +190,4 @@
* NOTE: This is not the correct normals.
* The correct smooth normals for the equator vertices should be
* {+-0.943608f * M_SQRT1_2, -0.331048f, +-0.943608f * M_SQRT1_2}
* but it creates problems for outlines when bones are scaled.
Why does it create problems? Are these new in this PR? Why didn't we have those problems before? Or did we, and nobody documented it?
😁 guess who didn't documented it?
So, yeah, I don't remember what it was (6y ago), but I don't want to try to change it now.
Don't remember
No, this comment is copied from
draw_cache.cc
line 1985/* creates problems for outlines when scaled */
I already added more context.
Before the original commit (6y ago) we didn't draw the outline the same way and there was no use of surface normals to move the outline edge slightly outwards.
So, yeah, I faced the issue, but did not document it properly. So I can't comment further.
@ -264,0 +415,4 @@
UNUSED_VARS(bone_octahedral_wire_lines);
/* Armature Octahedron. */
{
This is more of an overarching design question, but what's the reason this generic-looking shape cache needs such specific code for every Armature part? Wouldn't it be possible to have some {insert applicable design pattern here} approach to prevent specific changes to Armature drawing from rippling into more changes here?
In other words, this feels like too strong coupling between the armature specifics and the generic shape cache.
I am not sure I understand what you are trying to discuss. Better have a talk about it face 2 face.
TL;DR of the face-to-face: it's ok as-is, already a move forward. Future improvements are awaiting us in the future!
@ -482,3 +485,4 @@
BoneInstanceData() = default;
/* Constructor used by metaball overlays and expected to be used for drawing
* metaball_wire_sphere with armature wire shader that produces wide-lines. */
Is this comment still accurate? I can't find
metaball_wire_sphere
anywhere.@ -112,2 +114,4 @@
}
if (sub_object_id == uint(-1)) {
/* WORKAROUND: Armature code set the sub_object_id to -1 when individual bones are not
Why is the armature code setting what should be an unsigned integer to a negative value?
🤷 the -1 index is used to ignore the bone subobject selection. It should be a valid value but apparently it is not. It is a huge mess but eventually, we can clean that more later too.
@ -116,0 +124,4 @@
#ifdef DEBUG_PRINT
/* Print mapping from object name, select id and the mapping to internal select id.
* If something is wrong at this stage, it indicates an error in the caller code. */
So shouldn't this be an assertion then?
I meant if the values printed are not expected. There is not invalid values at this stage. If some values are out of range they are catched by the datastructures asserts.
@ -250,0 +270,4 @@
#ifdef DEBUG_PRINT
for (auto &hit : hit_results) {
/* Print hit results right out of the GPU selection buffer.
* If something is wrong at this stage, it indicates an error in the selection shaders. */
Same as above: shouldn't this be an assertion? Or accompanied by one?
Same as above reply.
When I say
indicates an error in the selection shaders
it usually means all values are 0. But that is still a valid selection result, so we cannot assert this.What does 'refreshing the viewport' mean?
I think it might be good to include such info in the PR description (or the parent PR).
is_overlay_next
member function. 63328dbe45clear_buffers
907c6833152bb4fff024
to21add52d7a
@blender-bot build