Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

propose architectural change with graphics object store #203

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pascal-niklaus
Copy link
Contributor

This PR does not yet contain code but contains a design document outlining an architectural change.

The idea is to discuss it, and once refined and agreed on, I would add actual code to this PR.

For ease of reading, I paste the content of the design document into this description:

Background and Rationale

gromit currently follows a "draw-and-forget" strategy, i.e. each
drawing operation, once completed, is drawn on a cairo_surface_t
(GromitData.back_buffer).

While this is very straightforward, it prevents the implementation of

  • dynamic content, e.g. items that fade after a certain time and
    disappear, or otherwise change through time

  • editing of past content (e.g. select and move or delete an old item)

Currently, a work-around exists in the form of an aux_backbuffer,
which stores the state prior to the drawing operation in
progress. This allows dynamic content during the ongoing drawing
operation, and is used in e.g. LINE, SMOOTH, RECT.
However, the drawing is split up over various callbacks (on_press,
on_motion, and on_release, which complicates maintenance).

Here, I propose to implement a "graphics object store" that contains
the elements that have been produced so far. By re-executing these
drawing operations (function redraw), the concent of backbuffer
could be re-created at any time.

Implementation

A graphics object store is added to GromitData. This object store
simple is a GList of GfxObject. Each GfxObject has the
following fields:

typedef struct {
    guint       id;                                 // unique id
    GfxType     type;                               // the type of object, e.g. Stroke, or Text
    gboolean    dynamic;                            // TRUE = content is dynamically updated
    gboolean    selected;                           // TRUE = object is currently selected
    guint32     capabilities;                       // bit mask with capabilities (see below)
    BoundingBox extent;                             // the objects rectangular extent
    bool        (*do)(GfxObject *, action, void *); // object-specific methods
    // ------------- object-specific fields -----------
    ...
} GfxObject;

The object specific fields allow for an inheritance-like structure,
i.e. each specialized object can be downcast to the above GfxObject.

For a normal stroked path, the specific field could be:

typedef struct {
    .... common fields ....
    //  ------------- object-specific fields -----------      
    GList     *coordinates;
    GdkRGBA    color;
    gfloat     arrowsize;
    ArrowType  arrowtype;
} GfxObjectPath;

For a text object, one could have:

typedef struct {
    .... common fields ....
    //  ------------- object-specific fields -----------      
    GdkRGBA  color;
    gchar   *text;
    gfloat   fontsize;
    gfloat   x, y;
}  GfxObjectPath;

A path that fades away after a certain time could look like so:

typedef struct {
    .... common fields ....
    //  ------------- object-specific fields -----------      
    GList     *coordinates;
    GdkRGBA    color;
    gfloat     arrowsize;
    ArrowTyype arrowtype;
    guint64    start_fading;  // timestamps in
    guint64    end_fading;    // microseconds
} GfxObjectFadingPath;

Capabilities

Each gfx_object has a bit mask indicating its "capabilities". These
capabilities imply that certain actions are implemented in the do
function.

Capability Description
draw object can be drawn
draw_transformed object can be drawn in transformed state;
this allows for dynamic representation during
operations such as dragging
move object can be moved
isotropic_scale object can be scaled isotropically
anisotropic_scale object can be scaled anisotropically,
i.e. differently in X and Y direction
isotropic_scale also must be set
rotate object can be rotated
edit object is editable (e.g. TEXT or parts of a path)
this probably is hardest to implement, and
left for the future

Use of capabilities with selections and during transformations

When multiple GfxObjects are selected, the capability bitmasks could
be anded to obtain the greatest common denominator. For example,
objects may be movable, but not rotateable, and then the rotate handle
is omitted in the GUI. Similarly, isotropic-only scaling would leave
only the corner handles, whereas anisotropic scaling would also
display handles in the middle of the sides (see
here for selection wireframe).

During transform operations, draw_transformed is called for each
object, with a 2D affine transformation matrix as argument.

At the end of the operation, transform is called with the final
transformation matrix, which then modifies the object in the object
store.

The 2D affine transformation functions are already implemented in
coordlist_ops.c.

Actions

When calling do, the action argument selects the task to be performed.

draw draw object in "normal" state
draw_selected draw object in "selected" state
draw_transformed draw object in intermediate "transformed" state;
this allows to display the object dynamically,
e.g. while it is being dragged
is_clicked detect whether a click at coordinate (x,y)
"hits" the object, for example to select it
transform transforms the object, modifying the context
in the gfx_store
destroy destroys the object, deallocating memory

Drawing of object store

Whenever objects need to be re-drawn, the redraw function simply iterates
through the object store and calls object->do(ptr, draw_action) for
all objects.

To improve update speed, a cached cairo_surface_t (similar to
backbuffer) is maintained. This cache contains all drawing
operations up to but not including the first that is flagged as
dynamic.

In practice, most of the time only the last object would need to be
redrawn atop the cached image. This is no different from the current
code which often draws over aux_backbuffer.

Migration to new drawing system

First step

  • Object store

    The object store is added to GromitData, together with some
    housekeeping functions.

  • Callbacks

    Then, the callbacks are adapted to add data to the object store
    instead of GromitData->coordlist. Direct drawing is replaced by a
    subsequent call of redraw.

    • on_press adds a new item of the respective type to the object
      store, and "remembers" its id (for example, in the hash_table).

    • on_motion appends coordinates to the respective GfxObject, and
      calls redraw.

    • on_release finalizes the operation, for example by smoothing a
      path (SMOOTH tool), and then calling redraw.

    • for each tool, the specific drawing operation is implemented. The
      only drawing operation that is required at this first stage is the
      drawing of a simple stroke path.

  • Undo/redo

    • "Undo" is implemented by simply moving the last GfxObjects
      from the object store to the head of a separate "redo" list,
      followed by a call to redraw.

    • "Redo" moves the first element from the "redo" list to the last
      position in the object store, followed by a call of redraw.

These are relatively minor changes. With these, the new architecture
already is functional.

Second step

In a next step, a "select" tool is implemented. This requires that the
GfxObject implement is_clicked and draw_selected.

The select tool draws a wireframe around the selection, showing the
respective handles (see "capabilities").

Then, a "delete" function is implemented. It basically calls the
"destroy" method and removes the selected items from the object store,
and then calls redraw.

Third step

Depending on the capabilities of the selected objects, suitable
transformations are offered. The simplest one is a "move" handle.

The items that implement "draw_transformed" are dynamically redrawn
during the "drag" operation. Upon releasing the "move" handle, the
tool calls the "transform" method, which modifies the coordinates of
the selecetd items in the object store.

Similarly, "scaling" and "rotation" operations are implemented by
adding extra handles to the selection wireframe. Everything else
remains the same.

Selection wireframe

The selection wireframe could look like this:

    R                                                 R
  R                                                     R
R   +---+                  +---+                  +---+   R
    | S |------------------| A |------------------| S |
    +---+                  +---+                  +---+
      |                                             |
      |                                             |
      |                                             |
      |                                             |
      |                                             |
      |                      ^                      |
    +---+                    |                    +---+
    | A |                <---M--->                | A |
    +---+                    |                    +---+
      |                      V                      |
      |                                             |
      |                                 BIN         |
      |                                 ICON        |
      |                                             |
      |                                             |
    +---+                  +---+                  +---+
    | S |------------------| A |------------------| S |
R   +---+                  +---+                  +---+   R
  R                                                     R
    R                                                 R
Handle Function
BIN trash icon, deletes the selected items
M move handle to drag the selection. SHIFT stays H or V
S isotropic scaling handle, opposite corner stays where it is
A anisotropic scaling handle, opposite side stays where it is
R rotate handle, opposite corner is center of rotation, SHIFT for 45° steps

@bk138
Copy link
Owner

bk138 commented Mar 28, 2024

Few thoughts:

Implementation

  • should be made into the form of an ADR eventually, see https://adr.github.io/ - I used https://github.com/npryce/adr-tools for tooling in other projects and would like to keep this - should be step 0 in the series of PRs
  • GList of GfxObject should be added to device data to have these per input device aka user in multi-user setups
    • OR have a GfxObject be tagged to which input device it belongs...
  • wouldn't all object types have all capabilities in the end? Or, put another way: is it only the fact that implementation effort is required that keeps object types from implementing an action or are there other, underlying, reasons?
  • not really clear to me yet what "transformed" state would be - can you please elaborate on that?
  • maybe gobject introspection can be used instead of capabilities (very crude thought, maybe it cannot...)
  • is_clicked is the only action which is not a verb - maybe get_is_clicked ?
  • how would the proposed list of GfxObject relate to undo/redo and the still present backbuffer?

Migration

  • would add a step 0 adding the design docs as ADR as outlined above
  • step 1: you mean "draw" instead of "redraw"?
  • undo/redo: would have these per device data, as outlined above

Selection wireframe

  • yes, lookgs good to me
  • BTW I found that the only tool i used so far that does everything right w.r.t. graphics drawing UX is Figma - have you tried it, what are your thoughts?

@pascal-niklaus
Copy link
Contributor Author

pascal-niklaus commented Mar 28, 2024

Implementation

* GList of GfxObject should be added to device data to have these per input device aka user in multi-user setups    
  * OR have a GfxObject be tagged to which input device it belongs...

I think with the latter it would be easier to maintain the "Z order" of items.

This then would mean that in a multi user environment, user can only manipulate their own objects? Similarly, for the undo/redo part? This would be different from the current system, right?

* wouldn't all object types have all capabilities in the end? Or, put another way: is it only the fact that implementation effort is required that keeps object types from implementing an action or are there other, underlying, reasons?
  • Step-wise implementation is one reason

  • There may be objects that have limited transformation capabilities. For example, some items may not be rotatable because it is too computationally costly, or some other items may only be isotropically scalable (maybe text). For example, for a future hypothetical quadrat tool, one may want to prevent it from being turned into a rectangle.

* not really clear to me yet what "transformed" state would be - can you please elaborate on that?

The idea is that, for example while an object is rotated, draw_transformed would leave the original object in the object store intact, but a call to draw_transformed(object, transformation_matrix) would draw the transformed object onto the screen. If an object cannot be dynamically re-drawn in the transformed state, only the selection wireframe would rotate visibly. When the rotate handle is released, the transformation matrix would be applied to the object (action transform), which then would modify the object in the object store.

This would allow to deal with objects that are computationally too "heavy" to transform and display dynamically

* maybe gobject introspection can be used instead of capabilities (very crude thought, maybe it cannot...)

I don't know enough about this.

* is_clicked is the only action which is not a verb - maybe get_is_clicked ?

Agree.

* how would the proposed list of GfxObject relate to undo/redo and the still present backbuffer?

My idea would be to still draw the objects into backbuffer, but maintain a cached version of the non-dynamic content.

  • below, A+B would be pre-drawn and stored in cache_buf
    draw would copy cache_buf to backbuffer and add C + D on top
    store -> obj_A -> obj_B -> obj_C[dyn] ->  obj_D -> nil
                        ^
                        |
                   cache_buf [contains A + B]

    redo -> nil
  • after undo, we would have this situation:
    store -> obj_A -> obj_B -> obj_C[dyn] 
                        ^
                        |
                   cache_buf [contains A + B]

    redo -> obj_D -> nil
  • after two more undos, we would have this situation:
    store -> obj_A 
               ^
               |
            cache_buf [contains A]

    redo -> obj_D -> obj_C[dyn] -> obj_B  

So basically after each change to the object store, one would check what the
last non-dynamic object is, and re-generate cache_buf when required.

The same would be true if for example a dynamic object "expires"
because it fades away and is deleted:

    store -> obj_A -> obj_B -> obj_C[dyn] ->  obj_D -> nil
                        ^
                        |
                   cache_buf [contains A + B]

    redo -> nil

when C disappears, one has:

    store -> obj_A -> obj_B -> obj_D -> nil
                                  ^
                                  |
                             cache_buf [contains A + B + D]

    redo -> nil

I have started to code a simple implementation to see how this might look like, and in this one I use the object's id to indicate the cache position, not pointers. Finding the items is very fast even with a simple linear search.

Thinking about it, I now realize that also the individual transformations need to be undo/redoable, not just the addition of a new object. Maybe this would require to make a deep copy of the object (or group of objects), and place it in the redo buffer, somehow "flagging" that undo would replace the existing objects (or their contents). Maybe the id could be used for this, but I need to think more about this.

Migration

* would add a step 0 adding the design docs as ADR as outlined above

* step 1: you mean "draw" instead of "redraw"?

Yes.

@bk138
Copy link
Owner

bk138 commented Mar 29, 2024

Implementation

* GList of GfxObject should be added to device data to have these per input device aka user in multi-user setups    
  * OR have a GfxObject be tagged to which input device it belongs...

I think with the latter it would be easier to maintain the "Z order" of items.

OK!

This then would mean that in a multi user environment, user can only manipulate their own objects? Similarly, for the undo/redo part? This would be different from the current system, right?

Good question! I think we should align with what makes sense. Proposal:

  • let undo/redo be user-specific
  • move/scale: might make sense to be able to manipulate other user's drawings?
  • eraser: definitely should be able to erase other user's drawings

AFAICS undo/redo is global now as is erasing. Only drawing is multi-user (LINE/RECT might not be as per their use of aux_backbuffer)

* wouldn't all object types have all capabilities in the end? Or, put another way: is it only the fact that implementation effort is required that keeps object types from implementing an action or are there other, underlying, reasons?
* Step-wise implementation is one reason

* There may be objects that have limited transformation capabilities. For example, some items may not be rotatable because it is too computationally costly, or some other items may only be isotropically scalable (maybe text).  For example, for a future hypothetical quadrat tool, one may want to prevent it from being turned into a rectangle.

So no "hard" reasons IMO ;-) We can go with the current idea, I just want to challenge the current reasoning.

* not really clear to me yet what "transformed" state would be - can you please elaborate on that?

The idea is that, for example while an object is rotated, draw_transformed would leave the original object in the object store intact, but a call to draw_transformed(object, transformation_matrix) would draw the transformed object onto the screen. If an object cannot be dynamically re-drawn in the transformed state, only the selection wireframe would rotate visibly. When the rotate handle is released, the transformation matrix would be applied to the object (action transform), which then would modify the object in the object store.

Mh why not dynamically redraw in transient transformed state always? Wouldn't that be what users expect from a "modern" UI? Drawing transient wireframes seems a bit like a 90's performance excuse that we don't really need anymore to me.

This would allow to deal with objects that are computationally too "heavy" to transform and display dynamically

See above. Does this really apply? I again want to point to figma (figjam there) who are doing it all in a browser without being slow.

* maybe gobject introspection can be used instead of capabilities (very crude thought, maybe it cannot...)

I don't know enough about this.

Me neither, it justed sounded suitable from the back of my head. Might need some reading. We can go without it if too much up-front effort.

* is_clicked is the only action which is not a verb - maybe get_is_clicked ?

Agree.

* how would the proposed list of GfxObject relate to undo/redo and the still present backbuffer?

My idea would be to still draw the objects into backbuffer, but maintain a cached version of the non-dynamic content.

* below, A+B would be pre-drawn and stored in `cache_buf`
  `draw` would copy `cache_buf` to `backbuffer` and add C + D on top
    store -> obj_A -> obj_B -> obj_C[dyn] ->  obj_D -> nil
                        ^
                        |
                   cache_buf [contains A + B]

    redo -> nil
* after undo, we would have this situation:
    store -> obj_A -> obj_B -> obj_C[dyn] 
                        ^
                        |
                   cache_buf [contains A + B]

    redo -> obj_D -> nil
* after two more undos, we would have this situation:
    store -> obj_A 
               ^
               |
            cache_buf [contains A]

    redo -> obj_D -> obj_C[dyn] -> obj_B  

So basically after each change to the object store, one would check what the last non-dynamic object is, and re-generate cache_buf when required.

The same would be true if for example a dynamic object "expires" because it fades away and is deleted:

    store -> obj_A -> obj_B -> obj_C[dyn] ->  obj_D -> nil
                        ^
                        |
                   cache_buf [contains A + B]

    redo -> nil

when C disappears, one has:

    store -> obj_A -> obj_B -> obj_D -> nil
                                  ^
                                  |
                             cache_buf [contains A + B + D]

    redo -> nil

I have started to code a simple implementation to see how this might look like, and in this one I use the object's id to indicate the cache position, not pointers. Finding the items is very fast even with a simple linear search.

Thinking about it, I now realize that also the individual transformations need to be undo/redoable, not just the addition of a new object. Maybe this would require to make a deep copy of the object (or group of objects), and place it in the redo buffer, somehow "flagging" that undo would replace the existing objects (or their contents). Maybe the id could be used for this, but I need to think more about this.

Likewise, will provide feedback on this in a later edit.

Migration

* would add a step 0 adding the design docs as ADR as outlined above

* step 1: you mean "draw" instead of "redraw"?

Yes.

@pascal-niklaus pascal-niklaus force-pushed the devel_object_store branch 3 times, most recently from bed8d6d to 99614d5 Compare March 30, 2024 10:40
@pascal-niklaus
Copy link
Contributor Author

I have now refined the proposal and added it as ADR, under doc/architecture/decisions/0002.

In particular, it now contains an undo/redo mechanism that should work with multiple users.

Please comment.

@bk138 bk138 self-assigned this Mar 30, 2024
Copy link
Owner

@bk138 bk138 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2nd round of thoughts.


- each device (i.e. user) contains an `GList` with `undo` and `redo` records.

- `GromitData` is extended with an undo object store (`undo_objects`).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix w/ "The global " to make this clearer please.

- any non-undo/redo operation clears the `redo` records.

The `undo` records contain the `id` of the object to move from
`objects` to `undo_objects`, and the `id` of the object to move from
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it clear that objects is in GromitData and "global" please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is written on line 35 already...

selection backwards in Z-order?

- Orphans could occur in the undo store. One possibility would be to
reference-count these objects and delete them when the last
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@pascal-niklaus pascal-niklaus Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but then pointers are required. I prefer to have the object ids. And since the objects are part of a list, they should not be deallocated, but the list link needs to be deleted.

@pascal-niklaus
Copy link
Contributor Author

Some more thoughts:

RECOLOR will have to go, I think.

ERASER should go as well, I think. One could of course draw with a corresponding pen, but maybe it is better do delete entire objects with the new mechanism, or just "undo" whenever possible.

  • ERASER would rely on a stable Z-order (which could become a problem when multiple users edit simultaneously)

  • ERASER would produce "hidden" object parts, which would interfere with object selection (one could click on object parts that are invisible).

@bk138
Copy link
Owner

bk138 commented Apr 9, 2024

Some more thoughts:

RECOLOR will have to go, I think.

ERASER should go as well, I think. One could of course draw with a corresponding pen, but maybe it is better do delete entire objects with the new mechanism, or just "undo" whenever possible.

* ERASER would rely on a stable Z-order (which could become a problem when multiple users edit simultaneously)

* ERASER would produce "hidden" object parts, which would interfere with object selection (one could click on object parts that are invisible).

RECOLOR probably does not matter, but i do know that people use ERASER, so we need to find a solution for this one. It could

  • simply modify backbuffer and not have any undo/redo to it
  • or be like a PEN but destructive

@pascal-niklaus
Copy link
Contributor Author

pascal-niklaus commented Apr 9, 2024

... but i do know that people use ERASER, so we need to find a solution for this one. It could

* simply modify backbuffer and not have any undo/redo to it

There is no backbuffer anymore, objects are drawn directly...

* or be like a PEN but destructive

Yes, that would work, but see above (Z-order, destructive). As an intermediate fix, this certainly would be ok and meet some user's demands.

However, people currently use ERASER because there is no other possibility to delete (parts of) an object.

IMO the best solution would be to find another, intuitive use of e.g. the back of a tablet pencil. One could think about selecting an object by tapping on it, and then select the part of the path one wants to delete. This could either split the object into multiple new objects, or add sections with width = 0 that are skipped during drawing of the object-.

What I also want to do is to think about "on the fly" ways to correct drawings.

Here is an example with which I am experimenting in the "orthogonal" tool, where "back-tracking" can be used to correct the drawing. This is already with the new object store system, so direct drawing without any backbuffer.

ortho_new

@bk138
Copy link
Owner

bk138 commented Apr 10, 2024

... but i do know that people use ERASER, so we need to find a solution for this one. It could

* simply modify backbuffer and not have any undo/redo to it

There is no backbuffer anymore, objects are drawn directly...

Hmm, there was a buffer mentioned on the very top of this PR at "Drawing of object store". In case the design in the ADR departs from that (and recreates the to-be-drawn-buffer from objects each time?), why not get back to this? OTOH, I probably have to get my head around the fact that the new design changes the program from a raster drawing tool to a kind of vector drawing tool with objects. Would this describe the new UX?

What I also want to do is to think about "on the fly" ways to correct drawings.

I think while this might be neat technically, it's not really useful UX and kinda goes over the top of the functionality of an annotation tool :-)

@pascal-niklaus
Copy link
Contributor Author

Hmm, there was a buffer mentioned on the very top of this PR at "Drawing of object store". In case the design in the ADR departs from that (and recreates the to-be-drawn-buffer from objects each time?), why not get back to this? OTOH, I probably have to get my head around the fact that the new design changes the program from a raster drawing tool to a kind of vector drawing tool with objects. Would this describe the new UX?

Yes, the buffer was more to cache the drawing. But I think you said that this was rather "90s" or something like that, and I think you were right.

Only the objects that are in the area that is invalidated are redrawn. The code called from "on_expose" looks like this:

void draw(GromitData *data, cairo_t *cr) {
    GdkRectangle clip_rect;
    gdk_cairo_get_clip_rectangle(cr, &clip_rect);
    for (GList *ptr = data->objects; ptr; ptr = ptr->next) {
        GfxObjBase *obj = ptr->data;
        if (rects_overlap(&clip_rect, &obj->bbox))         // <=== here is the check 
            if (obj->capabilities & CAP_DRAW)
                obj->exec(data, obj, ACTION_DRAW, cr, NULL);
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants