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

reimplement Image Icon's in v7 & FontBook class #80

Merged
merged 48 commits into from
Feb 26, 2024
Merged

Conversation

mggower
Copy link
Collaborator

@mggower mggower commented Nov 2, 2023

No description provided.

@mggower mggower changed the title reimplement Image Icon's in v7 reimplement Image Icon's in v7 & FontBook class Nov 6, 2023
Base automatically changed from feature/label-styles to master November 6, 2023 20:56
Copy link
Contributor

@jameslaneconkling jameslaneconkling left a comment

Choose a reason for hiding this comment

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

In order for this to work w/ the image renderer, you'll need to track which remote assets are currently being loaded, and wait to generate the image until after they're rendered. otherwise image exports will be missing icons/labels. also, need to cancel the load task in the event that the node or renderer is destroyed. in v6 I managed this via a callback util called Async.

It's possible that in v7, pixi can now handle this natively via https://pixijs.com/guides/components/assets. Not sure...

src/renderers/webgl/textures/icons.ts Outdated Show resolved Hide resolved
src/renderers/webgl/index.ts Show resolved Hide resolved
src/renderers/webgl/textures/icons.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jameslaneconkling jameslaneconkling left a comment

Choose a reason for hiding this comment

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

Lot going on here, so if any of this sounds wrong to you, tell me, definitely not completely confident in everything below.

One source of bugs early on in the previous version was that promises can't resolve synchronously and also can't be cancelled.

Because they can't resolve synchronously, if you create a cache for something like a BitmapFont or image texture, and a getter function returns a promise that immediately resolves in case of a cache hit, the getter will still resolve in the next tick, not synchronously in the same tick (this is by design in the promise spec, and for the life of me I don't know why). This is usually bad, b/c any logic in the then clause won't run until after the current render completes w/ the call to this.app.render().

Because promises can't be cancelled, there's a potential race condition where if you request a texture be created asynchronously, then tear down the Trellis instance before the texture is created, when the promise resolves and the pixi app has been destroyed, trying to create that texture will both throw an error and leak memory.

A final bug I kept running into: the image renderer needs to first render the scene, and if there are remote assets still being loaded, wait for them to load and be rendered before rendering the image.

It's possible to work around these limitations: for the first, have a synchronous cache check that you always use before loading anything. For the second, always check if the app has been destroyed before doing anything pixi related in a then clause. And for the third, check the loading status on all objects. But ultimately this was pretty brittle, so in the previous version I created the Loader abstraction that's backed by this Async util. (it's kind of like a functional promise: it's a higher order function, not a class, and it's synchronous, and cancellable)

My suggestion here would be:

  • create webgl/loader/imageLoader.ts, webgl/loader/fontLoader.ts, replacing webgl/textures/font, webgl/textures/icons (decoupling the loading/cacheing of external assets from the creation of image textures, and from the creation of icon or label objects).
  • use the loaders something like here before creating any objects (b/c the loaders are synchronous, the callback is called immediately if there's a cache hit).
  • cancel loaders when deleting a node/edge (this will noop if all loaders have already resolved).
  • once we implement the image renderer, use the loader singletons so the image renderer can track the number of assets still loading.

Copy link
Contributor

@jameslaneconkling jameslaneconkling left a comment

Choose a reason for hiding this comment

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

Also, might make sense to roll up #94 into this PR

dependabot bot and others added 3 commits February 6, 2024 12:46
Bumps [msgpackr](https://github.com/kriszyp/msgpackr) from 1.9.9 to 1.10.1.
- [Release notes](https://github.com/kriszyp/msgpackr/releases)
- [Commits](kriszyp/msgpackr@v1.9.9...v1.10.1)

---
updated-dependencies:
- dependency-name: msgpackr
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 4.5.0 to 4.5.2.
- [Release notes](https://github.com/vitejs/vite/releases)
- [Changelog](https://github.com/vitejs/vite/blob/v4.5.2/packages/vite/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite/commits/v4.5.2/packages/vite)

---
updated-dependencies:
- dependency-name: vite
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>
@mggower
Copy link
Collaborator Author

mggower commented Feb 6, 2024

added edge labels in this PR

Copy link
Contributor

@jameslaneconkling jameslaneconkling left a comment

Choose a reason for hiding this comment

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

Left some more suggestions. Overall general theme is that we should strive to keep the NodeRenderer/EdgeRenderer as simple as possible by pushing logic to the objects and textures. NodeRenderer/EdgeRenderers and the parent renderer are already the location of most of the complexity, so anything like asset loading and texture creation/caching/management that can be pushed to the object/texture classes could be a big win.

I was having a harder time reasoning about the behavior and perf implications of the loaders themselves. The things to confirm is that

  • rendering a graph w/ 50k nodes/edges doesn't degrade performance in comparison to what's currently on master
  • renderer behavior is the same regardless of whether or not a loader is used to load a remote asset (e.g. font: 'sans-serif' which uses a native font, and font: 'roboto' which requires loading a remote font, should both produce equivalent graphs)
  • it's possible to know whether remote assets are currently being loaded (so that the image renderer knows to not capture the image until after assets have loaded), and possible to cancel all requests for assets (so that we don't try to create pixi objects after the renderer has been torn down).

src/renderers/webgl/loaders/FontLoader.ts Outdated Show resolved Hide resolved
src/renderers/webgl/edge.ts Outdated Show resolved Hide resolved
src/renderers/webgl/node.ts Outdated Show resolved Hide resolved
src/renderers/webgl/node.ts Outdated Show resolved Hide resolved
src/renderers/webgl/node.ts Outdated Show resolved Hide resolved
src/renderers/webgl/node.ts Outdated Show resolved Hide resolved
src/renderers/webgl/textures/font.ts Outdated Show resolved Hide resolved
src/renderers/webgl/textures/textIcon/TextIconCache.ts Outdated Show resolved Hide resolved
@mggower mggower merged commit a9b2475 into master Feb 26, 2024
2 checks passed
@mggower mggower deleted the feature/assets branch February 26, 2024 11:39
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