Page MenuHome

Transform: Split TransData struct in different arrays
Changes PlannedPublic

Authored by Germano Cavalcante (mano-wii) on Jun 4 2020, 7:59 PM.

Details

Summary

Overview
This is an alternative and more conventional solution for D7902

Basically each component of the old TransData is now separated
into more specific structs.

Motivation
This patch was originally developed as a step to fix T70873 more
appropriately (reusing the struct TransData for mirror elements in
order to deduplicate code and simplify use).

(I was impatient and committed another solution for it).

Even though that bug has been solved, this patch still points out a
problem in the transform code, which is memory usage.

TransData is a struct allocated for each element affected in the
transformation.

When proportional edit is enabled, all elements visible in the scene
have a respective TransData allocated.

This indicates that there is a TransData for each vertex, object,
particle or whatever.

However, TransData (in conjunction with TransDataExtension) has a
total of 41 members and a size of 608 bytes!

But for most transformations, we only need 24 bytes (int flag, float *loc, float iloc[3])

So we only need about 4% of the total size of the TransData struct for most of the transform modes.

That struct is oversized, and is an unnecessary waste of memory.

Results
At the moment some arrays of unused structs are being allocated together.
They will be removed later if the idea of this solution is accepted.

A benefit of this patch at the moment is the performance, which even
though it does not save memory still presents some difference (~20%):

Translate:Single Struct:Splitted Struct:
TIME CONVERT0.0926060.058717
TIME MODE0.0449190.039117
TIME END0.0632590.050652
Rotate:Single Struct:Splitted Struct:
TIME CONVERT0.0935240.058600
TIME MODE0.0605360.048597
TIME END0.0577280.059498
Scale:Single Struct:Splitted Struct:
TIME CONVERT0.0902810.059215
TIME MODE0.0650830.062000
TIME END0.0629250.048495

Diff Detail

Repository
rB Blender
Branch
transform_data_struct_of_arrays (branched from master)
Build Status
Buildable 8380
Build 8380: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Jun 4 2020, 7:59 PM
Germano Cavalcante (mano-wii) created this revision.
  • Fix wrong checks of the existence of 'tdi'

Before getting into detailed review, I'd like to check with other developers.

Some more general points:

  • I find overall the readability of the code a little worse, you always need to beware that the second index needs to be used td->center can't be used as is, without td->center[index].

    Statements such as:

    copy_m3_m3(td->axismtx, td_table[td_index]->axismtx); get an extra level of indirection, becoming.

    copy_m3_m3(td->space[tdi].axismtx, td->space[td_table[td_index]].axismtx);
  • We loose some type safety, passing int's between functions can be more easily swapped out with other arguments (boolean or other int's) without any errors, besides runtime errors.
  • Before creating this patch you were looking into adding data which would have bloated the transform struct. I'd be interested to see a patch for this, showing the pain-point that this patch intends to solve, to see the trade off being made.
  • While it's nice there is a speedup, this is a modest gain for a one-time cost, which I would guess isn't all that bad compared to updating the depsgraph & 3D viewport.
source/blender/editors/transform/transform_data.c
126–140

Couldnt BLI_array_permute_ex be used here?

I do not have a strong opinion on this patch… Main points I see from a quick check:

  1. Performances argument should be ignored imho, this is at best marginal improvement in the overall picture.
  2. Readability: am not sure... in a way, passing around explicit index of each item that need to be processed makes thing a bit more clear? But it also does add some bloating…
  3. Memory usage control is the main strong point here, would be interesting to have metrics about that?

(...)

  • Before creating this patch you were looking into adding data which would have bloated the transform struct. I'd be interested to see a patch for this, showing the pain-point that this patch intends to solve, to see the trade off being made.

(...)

I actually wanted to use the same struct TransData as TransDataMirror.
But another solution was commited (rB1c189aa70aff)
However the "void *extra" and "float center[3]" members should be optional

I think this makes sense if we can improve readability and type safety.

  • Rename basic to base. ext and special could also get better names?
  • Putting an index in base would help with type safe, you'd pass the base between functions then.
  • Use more temporary variables rather than indexing multiple times.

So for example:

TransData *td = tc->data;
for (int tdi = 0; tdi < tc->data_len; tdi++) {
  if (td->basic[tdi].flag & (TD_MIRROR_EDGE_X | TD_MIRROR_EDGE_Y | TD_MIRROR_EDGE_Z)) {
    if (td->basic[tdi].flag & TD_MIRROR_EDGE_X) {
      td->basic[tdi].loc[0] = 0.0f;
    }
    if (td->basic[tdi].flag & TD_MIRROR_EDGE_Y) {
      td->basic[tdi].loc[1] = 0.0f;
    }
    if (td->basic[tdi].flag & TD_MIRROR_EDGE_Z) {
      td->basic[tdi].loc[2] = 0.0f;
    }
  }
}

To:

TransDataBase *tbase = td->base;
for (i = 0; i < tc->data_len; i++, tbase++) {
  if (tbase->flag & (TD_MIRROR_EDGE_X | TD_MIRROR_EDGE_Y | TD_MIRROR_EDGE_Z)) {
    if (tbase->flag & TD_MIRROR_EDGE_X) {
      tbase->loc[0] = 0.0f; 
    }
    if (tbase->flag & TD_MIRROR_EDGE_Y) {
      tbase->loc[1] = 0.0f; 
    }
    if (tbase->flag & TD_MIRROR_EDGE_Z) {
      tbase->loc[2] = 0.0f; 
    }
  }

  TranseDataSpecial *special = &td->special[tbase->index];
  ...
}
Germano Cavalcante (mano-wii) planned changes to this revision.Thu, Jun 18, 8:01 PM

The transform code is very large and these changes are quite laborious.
I appreciate the review, I now know that this idea can go ahead with the suggested changes :)

But before I continue this code, I will try to simplify the code in other ways:

  • Move some operators out of the transform code.
  • Unify some similar members.
  • Deduplicate code that is repeated in some transform modes.

This will help to have a clearer picture of how this can be done.