Details
Diff Detail
- Repository
- rB Blender
- Branch
- fix-rearrange-gpencil-channels (branched from master)
- Build Status
Buildable 23156 Build 23156: arc lint + arc unit
Event Timeline
| source/blender/editors/animation/anim_channels_edit.c | ||
|---|---|---|
| 1505–1507 | It's not entirely clear to me why this has to do yet another check on ac.datatype. The above already has a chain-of-if-else that checks on those, and below there is also a check on ac.datatype inside the for-loop. The code now seems to do two loops over the animation data, with one loop handling a subset of the items, and the other loop the other subset. Would it be possible to extract the inner part of the for-loop of rearrange_gpencil_channels() into its own function, then call that from both loops? That way animchannels_rearrange_exec() wouldn't have to call rearrange_gpencil_channels(), which cut the number of loops in half. | |
| source/blender/editors/animation/anim_channels_edit.c | ||
|---|---|---|
| 1505–1507 | The issue is that in current state, the loop in rearrange_gpencil_channels needs to have ANIMFILTER_LIST_CHANNELS, in order to deal with ANIMTYPE_DSGPENCIL(grease pencil containers in the dopesheet) and rearrange among them BUT adding this filter to the main dopesheet loop also embeds other channel types (including summary), which are not treated well by the call to rearrange_action_channels and lead to a crash. That being said, I agree that this the current solution not ideal, I am going to think of a cleaner way to deal with this. | |
That being said, I agree that this the current solution not ideal, I am going to think of a cleaner way to deal with this.
Did you find a cleaner way of doing this? Or is this something that we should land as it is now (maybe with a bit more explanation in a comment)?
I took a second look at this today. I don't see a nice/one-loop way of doing this.
If we feel it's not good enough to be committed, I won't argue against leaving the issue pending until we find a better solution. There are other ways of rearranging layers of a grease pencil object.
We may get to a cleaner solution if we decide to group all layers channels within a sub-container, which would separate them from other gpencil animated data. This container could be of type ANIMTYPE_GPDATABLOCK, like in the Grease Pencil mode of the Dopesheet.
I guess that is a design discussion that we would need to have separately from this rearranging issue ?
I guess that is a design discussion that we would need to have separately from this rearranging issue ?
I agree, so let's get this in first.
Well, yes but also isn't it a bit late ?
It's not exactly a bug fix, I see it's bcon3 already.
Same goes for D16369 IMO.
OKi (was under the impression D16369 would have counted as fix, but "better safe than sorry", so lets leave as-is)