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

LOD: make a hook available to rewrite image urls as needed #3764

Closed
wants to merge 1 commit into from

Conversation

mimecuvalo
Copy link
Collaborator

This continues the idea kicked off in #3684 to explore LOD and takes it in a different direction.

Using the work in #2825 that (in one of the commits) introduced the concept of providing overridable hooks (in the composable UI family), this provides a hook to rewrite asset url's as necessary, given current context.

Several things here to call out:

  • the regular hook is just a pass-through function
  • our dotcom version would start to use Cloudflare's image transforms
  • we don't rewrite non-image assets
  • we debounce zooming so that we're not swapping out images while zooming (it creates jank)
  • we load different images based on steps of .25 (maybe we want to make this more, like 0.33). Feels like 0.5 might be a bit too much but we can play around with it.
  • we take into account network connection speed. if you're on 3g, for example, we have the size of the image.
  • dpr is taken into account - in our case, Cloudflare handles it. But if it wasn't Cloudflare, we could add it to our width equation.
  • we use Cloudflare's fit=scale-down setting to never scale up an image.
  • we don't swap the image in until we've finished loading it programatically (to avoid a blank image while it loads)

We need to enable Cloudflare's pricing on image transforms btw @steveruizok πŸ˜‰ - this won't work quite yet until we do that.

Change Type

  • sdk β€” Changes the tldraw SDK
  • dotcom β€” Changes the tldraw.com web app
  • docs β€” Changes to the documentation, examples, or templates.
  • vs code β€” Changes to the vscode plugin
  • internal β€” Does not affect user-facing stuff
  • bugfix β€” Bug fix
  • feature β€” New feature
  • improvement β€” Improving existing features
  • chore β€” Updating dependencies, other boring stuff
  • galaxy brain β€” Architectural changes
  • tests β€” Changes to any test code
  • tools β€” Changes to infrastructure, CI, internal scripts, debugging tools, etc.
  • dunno β€” I don't know

Test Plan

  1. Test images on staging, small, medium, large, mega
  2. Test videos on staging
  • Unit Tests
  • End to end tests

Release Notes

  • LOD: add overridable hook to be able to rewrite asset urls.

Copy link

vercel bot commented May 17, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
examples βœ… Ready (Inspect) Visit Preview May 17, 2024 8:46am
1 Ignored Deployment
Name Status Preview Updated (UTC)
tldraw-docs ⬜️ Ignored (Inspect) May 17, 2024 8:46am

@huppy-bot huppy-bot bot added improvement Improves existing features sdk Affects the tldraw sdk labels May 17, 2024
useValue('zoom level', () => zoomUpdater.current(editor.getZoomLevel()), [editor])
const networkEffectiveType: string | null =
'connection' in navigator ? (navigator as any).connection.effectiveType : null
const dpr = window.devicePixelRatio
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have the DPR on the instance state, so could could use that instead in a little useValue. It's nice for cases when the DPR changes, for example a user moves a tab onto a screen with a different DPR.

Comment on lines +75 to +77
const [debouncedZoom, setDebouncedZoom] = useState(this.editor.getZoomLevel())
const zoomUpdater = useRef(debounce((zoom: number) => setDebouncedZoom(zoom), 500))
useValue('zoom level', () => zoomUpdater.current(this.editor.getZoomLevel()), [this.editor])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a fun / clever way of writing a debounced reactor. attn @ds300

We did something similar in useZoomCss using an effect scheduler. I'm not sure whether it matters one way or the other, but seeing this kind of "debounced reactor" pattern is used twice then maybe it's something to add to the lib.

@steveruizok
Copy link
Collaborator

At first my "no new APIs for one-offs" alarm was ringing, but the more I thought about this PR, it's really about creating an API for providing options to shapes, right?

Like in a less declarative setting I might be writing something like:

editor.registerShapeUtil(ImageShapeUtil, { overrideAssetSrc: mySpecialLodThing })

I see declarative APIs for this in bundlers, the X | [X, OptionsForX] format, which I'd been considering for us to let users make customizations for tools:

<Tldraw
  tools=[SketchTool, [BalloonTool, { popPressure: 2 }]
  shapes=[SketchShape, [BalloonShape, { popSound: "/pop.mp3" }]
/>

For this case the hooks API you're proposing would be much easier than the above, but it feels more like I'm providing a general replacement for a piece of behavior, or a more general option for the editor, rather than adding some customization to a shape.

One other question I have is, do these have to be hooks that we're passing in? ie are they tied to the React lifecycles?

Providing config to our shapes is probably a bigger question, and since this is technically a one-off for now, it's worth also thinking of other ways to hack around this without adding an API. I'm a bit reminded of the whole "double click on canvas to create a text shape" behavior, which is sometimes not wanted, so we have a member method on the Select idle tool. Our current advice is "yeah replace that method at runtime with () => {}". We can't exactly do that for rendering because the shapes will render immediately.

Is there a shitty way of achieving this kind of dotcom-specific logic without creating a new public API?

@SomeHats
Copy link
Contributor

Sort of commenting with some general thoughts on both this and #3745.

I think the thing that we're sort of dancing around with both of these PRs is the fact that efficient asset storage and retrieval is extremely contextual: for dotcom local rooms we'd want blob assets. For dotcom multiplayer we want them on cloudflare, with some server-side opimisation and resizing. For a customer just prototyping tldraw with yjs, they probably want data URLs because they 'just work', although that isn't what they'd eventually deploy. For a customer with high sensitivity requirements, they'd need a system for signing asset requests to make sure that they're not publicly accessible.

We have some of this already: our registerExternalAssetHandler allows devs to change where their assets are stored, but they're restricted to our output format: just a single URL.

This hook starts to get us towards having something for how we read those assets. Right now it's very LOD focused, but I sort of think we want something like this for assets in general. That version couldn't be replaced by shape util options, because it's more about how assets integrate with tldraw as a whole.

Not sure exactly what this looks like, but one example (extremely BK) might be this:

const assets = {
    // the create callbacks are something similar to our current `registerExternalAssetHandler`
    // they're all optional, but would let you do something like pre-baking LODs
    create: {
        image: (file) => /* ... */,
        video: (file) => /* ... */,
        default: (file) => /* ... */,
    },
    // the resolve callbacks are used at runtime to turn the stored asset information into
    // the actual URL to use on the page.
    resolve: {
        default: (asset) => {
            // the default is something like this
            return asset.props.url
        },
        image: (asset, opts) => {
            // but you could also use them for variants like this
            return getScaledCdnImage(asset.props.url, opts.scale)
        },
        video: async (asset) => {
            // or to store the assets elsewhere like this
            return await loadImageFromBlobStore(asset.props.url)
        },
    }
}

// the assets object gets passed in like this
<Tldraw assets={assets} />

Another option is maybe something like an AssetUtil system? if you want your image to be stored elsewhere, you have to define your own special image asset util maybe?

@mimecuvalo
Copy link
Collaborator Author

Great feedback, you two!

At first my "no new APIs for one-offs" alarm was ringing, but the more I thought about this PR, it's really about creating an API for providing options to shapes, right?

That's suuuper interesting actually. I think that's definitely worth a discussion for this route you're proposing. Like you mention below about the case of disabling double-click on canvas for text shape, there does seem to be something speaking to a need of providing some configurability to shapes.

Is there a shitty way of achieving this kind of dotcom-specific logic without creating a new public API?

Haha, most definitely. But are we trying to solve this for dotcom or for our SDK users. For dotcom, for sure, we can hack something in, but it felt like this was more about serving a customer need.

I think the thing that we're sort of dancing around with both of these PRs is the fact that efficient asset storage and retrieval is extremely contextual: for dotcom local rooms we'd want blob assets. For dotcom multiplayer we want them on cloudflare, with some server-side opimisation and resizing. For a customer just prototyping tldraw with yjs, they probably want data URLs because they 'just work', although that isn't what they'd eventually deploy. For a customer with high sensitivity requirements, they'd need a system for signing asset requests to make sure that they're not publicly accessible.

It's a fair point! Although I would debate whether customers really want data URLs β€” they might work but the cost of simplicity is definitely an over-inflated store, and as you mention, it isn't what they would want in production anyway.

Signing asset requests feels like that could be achieved with the mechanism(s) we have? As you bring up, registerExternalAssetHandler could definitely do some S3 signing of assets if someone were inclined to do so. This hook though specifically is less about uploading/signing assets and just about rewriting URLs on demand, given a specific context.
I think maybe you're speaking more to the asset blob PR for this comment, and they maybe feel somewhat related, but the asset stuff is more about our local needs vs. this dynamic URL rewriting stuff.

This hook starts to get us towards having something for how we read those assets. Right now it's very LOD focused, but I sort of think we want something like this for assets in general. That version couldn't be replaced by shape util options, because it's more about how assets integrate with tldraw as a whole.

If I understand your direction here, I think it would probably worth discussing more at length for sure. I would venture to say that maybe even something even more general like an objectStore, perhaps not even limited to assets, might be worth discussing if we go this route. I know this sounds unrelated but, say, in the future we have comments/annotations or some other external (so-to-speak) or orthogonal piece to the canvas, those also might get stored differently than the regular store.

Ok, anyway, let's discuss more at length! I'll setup a meeting.

@mimecuvalo mimecuvalo mentioned this pull request May 22, 2024
2 tasks
@SomeHats
Copy link
Contributor

Discussing offline sounds good! One thing i wanted to write down whilst i was thinking of it:

Signing asset requests feels like that could be achieved with the mechanism(s) we have? As you bring up, registerExternalAssetHandler could definitely do some S3 signing of assets if someone were inclined to do so.

Depending on how asset signing is implemented, this may not be the case. At a previous company, we didn't want URLs floating round that gave unlimited access to assets intended to be private. If you were to use registerExternalAssetHandler, you'd be stuck with a signed asset URL in the store that you couldn't e.g. revoke access from to certain users. The system there involved generating URLs on-demand that were only valid for a particular user for a few minutes.

@mimecuvalo
Copy link
Collaborator Author

superseded by #3827
thx folks!

@mimecuvalo mimecuvalo closed this May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves existing features sdk Affects the tldraw sdk
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants