When transforming multiple strips to limits of sequencer timeline they get
squashed into one slot.
Store selection minimum and maximum channel in TransSeq and limit
transformation so no strip can be transformed beyond timeline boundary
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
I am not really sure why altering channel of transformed strip avoids overlap? As in, what guarantees that changing transformed strip channel does not make it overlap with something what is at the "new" channel you are assigning?
| source/blender/editors/transform/transform_convert_sequencer.c | ||
|---|---|---|
| 68 | At least add a comment explaining what is stored in the elements of the array. Maybe for readability it even makes more sense to store explicit min and max values? | |
Comment Actions
It doesn't guarantee no overlapping, this change is merely to respect editor range in nicer way
Perhaps I shouldn't consider report a bug and this is not a bugfix really. Though I would still consider this to be an improvement
| source/blender/editors/transform/transform_convert_sequencer.c | ||
|---|---|---|
| 68 | I guess I could have wrap these values in struct, so I don't need 2 _get functions? | |
Comment Actions
Ok, so it's a limit enforcement during transform. Seems to be working fine.
Here is some code related feedback.
| source/blender/editors/transform/transform_convert_sequencer.c | ||
|---|---|---|
| 68 | The thing here is that it seems weird to require TransSeq be defined in a private place of some .c file, and implement accessory to individual fields. What would work better here is either of those:
| |
| 629 | You initialize min, but not max. This is a bit weird. | |
Comment Actions
Think within current header structure transform_data.h is a better place. Maybe even add a sequencer-specific header even.
To me it is more like a details where there is no clear the-only-answer because it feels like the transform code layout could be encapsulated into areas even further. Will leave final guidance to @Germano Cavalcante (mano-wii).
Comment Actions
I choose transform_convert.h because there are already custom data like TransMirrorData. I am not too familiar with layout of transform code in context of other editors too much. To me it looks like some clearer level of isolation would be nice though.
Comment Actions
I choose transform_convert.h because there are already custom data like TransMirrorData. I am not too familiar with layout of transform code in context of other editors too much. To me it looks like some clearer level of isolation would be nice though.
I see. A bit confusing. Lets wait advice from @Germano Cavalcante (mano-wii) (who, to my knowledge, was decoupling implementation in the area) about where best to put the struct.
Once you guys agree on this topic you can commit the fix. So since there is nothing from my side to be requested to be changed I'll accept from my side, but since we want feedback from Germano I'll mark him as blocking review.
Comment Actions
What was idealized was not to have any specific structs to treat some type of conversion as a special case.
Functions like transform_convert_sequencer_get_snap_bound were also something that would be good to avoid.
But since we already have these special functions being exposed, how about keeping something more standardized and instead of exposing a struct, do an function like: transform_convert_sequencer_channel_clamp(TransInfo *t, const int channel)?
Comment Actions
Not sure if I understand this correctly, Do you mean that sequencer module should handle this internally?
Functions like transform_convert_sequencer_get_snap_bound were also something that would be good to avoid.
But since we already have these special functions being exposed, how about keeping something more standardized and instead of exposing a struct, do an function like: transform_convert_sequencer_channel_clamp(TransInfo *t, const int channel)?
Sounds reasonable.
Comment Actions
The basic idea is that the "modes" in transform_mode_... operate on the "converted" structures in "transform_convert_..." (that is, operate basically on TransData and TransDataContainer).
So it is not desirable that mode is using something that is very specific to the "converted" object.
Ideally, such bounds should be calculated in initSeqSlide using the members of TransData.
But sometimes this rule is not followed in favor of performance and simplicity.
And if I'm not mistaken (it might be good to check), the TFM_SEQ_SLIDE mode is only used with sequencer strips.
So the patch seems fine.