-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
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 |
There was a problem hiding this comment.
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.
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]) |
There was a problem hiding this comment.
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.
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 <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? |
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 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 |
Great feedback, you two!
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.
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.
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,
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 Ok, anyway, let's discuss more at length! I'll setup a meeting. |
Discussing offline sounds good! One thing i wanted to write down whilst i was thinking of it:
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 |
superseded by #3827 |
this is take #2 of this PR #3764 This continues the idea kicked off in #3684 to explore LOD and takes it in a different direction. Several things here to call out: - 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) TODO - [x] We need to enable Cloudflare's pricing on image transforms btw @steveruizok 😉 - this won't work quite yet until we do that. ### Change Type <!-- ❗ Please select a 'Scope' label ❗️ --> - [x] `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 <!-- ❗ Please select a 'Type' label ❗️ --> - [ ] `bugfix` — Bug fix - [x] `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 - [x] Unit Tests - [ ] End to end tests ### Release Notes - Assets: make option to transform urls dynamically to provide different sized images on demand.
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:
fit=scale-down
setting to never scale up an image.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 SDKdotcom
— Changes the tldraw.com web appdocs
— Changes to the documentation, examples, or templates.vs code
— Changes to the vscode plugininternal
— Does not affect user-facing stuffbugfix
— Bug fixfeature
— New featureimprovement
— Improving existing featureschore
— Updating dependencies, other boring stuffgalaxy brain
— Architectural changestests
— Changes to any test codetools
— Changes to infrastructure, CI, internal scripts, debugging tools, etc.dunno
— I don't knowTest Plan
Release Notes