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

feat: image support #4011

Merged
merged 174 commits into from Oct 21, 2021
Merged

feat: image support #4011

merged 174 commits into from Oct 21, 2021

Conversation

dwelle
Copy link
Member

@dwelle dwelle commented Sep 27, 2021

fix #19, replaces #3424 (more or less completely rewritten)

Refactor & thorough description TBD.

notes

  • When you import an image onto the scene, we do 2 things with the file:

    1. Generate an id for the image so it can be referenced later. By default we use the SHA-1 hash of the file.

    2. Get the file's DataURL (it looks like this: data:${mimeType};base64,${base64_of_the_image}).

      We use this when rendering the images on canvas (there aren't many way to do that. One of them is to create a HTMLImageElement and draw it on canvas using context.drawImage(). To create that img element, we'll need to set its src to something. When dealing with local data, it can be set to DataURL).

      Another benefit of DataURL is it doesn't need to be re-encoded when saving to JSON, which means we can just store it e.g. as part of the exported json (.excalidraw) and not care about decoding on restore. (Note that base64 is around 33% larger than binary encoding — so we if we cared about size, we'd need to dispense with JSON files and start exporting binary files).

  • Image elements (ExcalidrawImageElement) do not contain the image data itself for at least 2 reasons:

    1. More than one identical image can be on scene at the same time (especially true when people start using SVGs as libraries) which would duplicate the amount of data.
    2. When syncing changes, or saving element data to server, we don't want to unnecessarily transfer the image data every time.
  • We store the image id to the excalidraw element (fileId), and the image's DataURL to App.files which is an object of Record<FileId, BinaryFileData> where BinaryFileData is an object containing the DataURL and some metadata for the file.

  • For persisting the image files locally in the browser we use IndexedDB because LocalStorage is way too small (5MB). For now we still keep using LocalStorage for non-image state because it's faster to read from LocalStorage than from IDB (for cold starts). We may decide to migrate to IDB all the way later.

  • For server-persisted images, we upload them to Firebase Storage (an object store), both for collab and shareLink cases.

  • One other thing: while we use DataURL when rendering, we don't want to create the img elements on every render. While pretty fast, it's an async process (you need to set the src and wait on load event). So we keep a app.imageCache which is a Map<FileId, HTMLImageElement>.

  • When uploading images to server, we compress them (usually saves ~10-40% — of the binary size, not the DataURL size, because on server we are storing binaries) and encrypt them with the same key we use for the scene. The encoding format is described in encode.ts (see compressData() and concatBuffers()).

  • ExcalidrawImageElement["status"] flag (initially set to pending) is a "hacky" solution to determine whether the file is already persisted to storage.

    • In collaboration this is used to determine whether remote clients should pull the image binary from server (initially, the elements are synced to them with pending status, and after the origin client uploads the images, it emits the elements again, but with saved status).
    • When we open/close the collab room, we reset all images' status to pending. This way we signal that they should be persisted again to the respective storage (Firebase/IDB).
    • To determine whether we should prevent unload when closing the tab.

    The solution is "hacky" because it's not quite correct that pending status means the file isn't persisted. It just means that the scene element itself is in unsaved state — we need not only the image DataURL be persisted, but also the elements' status be properly set to saved and saved/synced with other clients, hence why we also use the status to prevent unload, and not the the FileManager class' #savedFiles map.

todos

  • remove the debug comments/hardcoded ids (check stash)
  • make flip vertical/horizontal contextMenu action account for rotation
  • encode duplicate images to SVG only once
  • fix cursor preview for SVGs
  • support pasting SVG source as image
  • better error handling (and terminal state) for images during collab
  • make inserted images take less screen space
  • resize images on insert
  • images have incorrect and/or negative dimensions when inserted on > 100% zoom (sometimes even on 100% when drag-n-dropping)
  • rename ImageIdFileId
  • support copy/pasting images across instances
  • save file metadata alongside dataURL
    • when saving to IDB
    • when saving to firebase
  • review/rewrite handling of image.status
  • prevent unload if images not persisted (server or IDB)
  • support host apps to supply file id
  • stop removing images from IDB on isDeleted: true update, and instead prune dead images on cold load (or other time), to guard against false positive race conditions and false negatives. .keys()
    • stop tracking deleted files
  • 🐛 legacy FS doesn't notify app of dialog cancelation resulting in the image tool being left selected
  • mobile: insert image on screen directly when picking from fs
  • don't reinitialize inserted image if already exists in state.files
  • add cursor image-preview when placing an image (when picking from a file)
  • add support for shareable links
  • fix loading images on file open
  • factor out encryptData from firebase.ts
  • disable adding images to library until support added (see below)

before merging

  • implement image support in E+
    • image support for export to Excalidraw+
  • [MAYBE] move the Spinner commit into its own PR.

Future

  • rendering to svg doesn't account for image rotation/scale
  • rendering images to SVG doesn't render placeholders
  • image preview cursor is reset on certain actions like moving canvas
  • we shouldn't upload images before you physically place them on canvas
  • resizing images doesn't work on Brave (prod build only?)
  • prune or limit app.imageCache size to guard against memory leaks
  • improve cursor image-preview so that the image-preview contains a cursor crosshair
  • let users pick whether they wanna import as an image or as a scene (when embedded)
  • support importing multiple files
    • from filepicker
    • drag-n-drop
  • support images in library (requires library schema v2)
  • add reset to original aspect ratio to contextMenu for images
  • allow changing stroke/fill for SVG images
  • support drag-n-dropping image from a browser tab
  • support uploading images via url
    • detect pasting image-like URLs

heitara and others added 30 commits March 31, 2021 02:27
* master: (159 commits)
  fix: deselect elements on viewMode toggle (#3741)
  fix: allow pointer events for disable zen mode button (#3743)
  feat: expose fontfamily and refactor FONT_FAMILY (#3710)
  feat: Show active file name when saving to current file (#3733)
  feat: add hint around text editing (#3708)
  chore(deps-dev): bump ts-loader in /src/packages/excalidraw (#3716)
  chore(deps-dev): bump ts-loader in /src/packages/utils (#3712)
  chore(deps-dev): bump typescript in /src/packages/excalidraw (#3671)
  chore(deps-dev): bump @babel/plugin-transform-typescript (#3713)
  chore(deps-dev): bump @babel/preset-env in /src/packages/utils (#3714)
  chore(deps-dev): bump @babel/plugin-transform-async-to-generator (#3715)
  chore(deps): bump ws from 7.4.3 to 7.4.6 in /src/packages/excalidraw (#3665)
  chore(deps-dev): bump webpack in /src/packages/excalidraw (#3670)
  chore(deps): bump ws from 7.4.3 to 7.4.6 in /src/packages/utils (#3664)
  chore(deps-dev): bump autoprefixer in /src/packages/excalidraw (#3672)
  chore(deps-dev): bump webpack in /src/packages/utils (#3673)
  chore(deps-dev): bump @babel/preset-env in /src/packages/utils (#3675)
  feat: change library icon to be more clear (#3583)
  chore: Update translations from Crowdin (#3659)
  fix: use excal id so every element has unique id  (#3696)
  ...

# Conflicts:
#	package.json
#	src/components/App.tsx
#	src/data/restore.ts
#	src/element/collision.ts
#	src/element/types.ts
#	src/keys.ts
#	src/locales/en.json
#	src/renderer/renderElement.ts

- File data are encoded as DataURLs (base64) for portability reasons.

[ExcalidrawAPI](https://github.com/excalidraw/excalidraw/blob/master/src/packages/excalidraw/README.md#onLibraryChange):
Copy link
Member

Choose a reason for hiding this comment

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

onLibraryChange

Copy link
Member Author

Choose a reason for hiding this comment

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

onLibraryChange API didn't change?

@zsviczian
Copy link
Collaborator

Hi @dwelle,

Is it on purpose that files is not part of SceneData type?

excalidraw/src/types.ts

Lines 236 to 241 in c834bbd

export type SceneData = {
elements?: ImportedDataState["elements"];
appState?: ImportedDataState["appState"];
collaborators?: Map<string, Collaborator>;
commitToHistory?: boolean;
};

@dwelle
Copy link
Member Author

dwelle commented Oct 21, 2021

@zsviczian TBH I don't know what that type is for. We use it only in updateScene(). I can add support for updateScene() to take files — but I'm not sure how it should behave. Identically to addFiles() (= merge files), or replace files with supplied files?

The API isn't settled yet, which is why I'm holding off on these changes until we gather more data from early integrations.

@dwelle
Copy link
Member Author

dwelle commented Oct 21, 2021

Alright people, let's do this 🚀

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.

Import images
6 participants