metauni / metaboard

Multiplayer drawing boards for sharing knowledge in Roblox.
Mozilla Public License 2.0
28 stars 6 forks source link

Drawing Tasks generalisability #67

Closed blinkybool closed 1 year ago

blinkybool commented 1 year ago

From CONTRIBUTING.md

This is a fairly ad-hoc treatment of the different types of drawing tasks. Notice that FreeHand and StraightLine drawing tasks return a figure from their Render method (not a figureId -> figure entry), whereas Erase drawing tasks return a table with figureIds as keys and masks as values. Erasing is a very different beast from figure-drawing, so there's no obvious way of making the output of DrawingTask.Render uniform across different types of drawing tasks, while preserving the ability to quickly recognise when we don't need to update a PureFigure. This doesn't mean there's no natural way to do it.

If we don't have to preserve the ability to shortcut updates, then the behaviour of all types of drawing tasks could be made uniform by making them store a function that takes the figure table as an argument and returns a new one with whatever changes it wants to make (adding a new figure, or replacing a figure with one that has a different mask). Checking for equality between the new function and the old one will only tell us that either no figures need to be changed (if functions are equal) or some of them do but we can't know which.

So perhaps a drawing task should explicitly store a function per-FigureId. So every time a render occurs, for each figureId, we gather all of the functions for that figureId from all of the drawing tasks, and if they are equal to all of the previous functions, then we know we can shortcut the update.

What's the point of fussing over this? Well currently the behaviour of the Erase drawing task is fragmented between src/common/DrawingTasks/Erase.lua, and all of the various renderers that have to gather and apply the right mask to each figure when they encounter drawingTask.Type == "Erase". So if you want to implement another type of drawing task that affects other figures (not just drawing a new figure), then you have to implement the render stage behaviour and shortcut detection in every PureFigure component in the repo under an elseif drawingTask.Type == "OtherType" clause.

In summary, I think a drawing task should tell you which figureIds it affects, and for each figureId:

  1. How to update the figure at that figureId in the render step
  2. Some kind of reference for this updater (or its generating data) which is changed whenever the drawing task modifies it, and is unchanged when the drawing task only modifies the updaters for other figureIds.
blinkybool commented 1 year ago

I want to add that we should consider what kind of other drawing tasks we want to support. Some examples off the top of my head are

Critically, not all of these follow the "init(canvasPos), update(canvasPos), finish()" recipe that drawing and erasing follow. But they must be the same kind of thing as our existing drawing tasks for them to follow the same undo/redo behaviour.

blinkybool commented 1 year ago

The discussion on this conclude with the sentiment that we should leave drawing tasks how they are for now.