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

Add pdf support #5896

Merged
merged 33 commits into from
Jan 31, 2023
Merged

Add pdf support #5896

merged 33 commits into from
Jan 31, 2023

Conversation

johnshaughnessy
Copy link
Contributor

@johnshaughnessy johnshaughnessy commented Jan 14, 2023

Add support for loading pdfs with bitecs.

Also rework coroutines by introducing JobRunner.

@johnshaughnessy johnshaughnessy force-pushed the feature/bit-pdf branch 2 times, most recently from 81b742c to abd9d21 Compare January 20, 2023 19:43
@johnshaughnessy johnshaughnessy changed the base branch from master to bug/2022-01-20 January 20, 2023 19:44
@johnshaughnessy johnshaughnessy force-pushed the feature/bit-pdf branch 2 times, most recently from 92164be to 536267a Compare January 20, 2023 19:55
@johnshaughnessy johnshaughnessy marked this pull request as ready for review January 20, 2023 20:02
@johnshaughnessy johnshaughnessy requested review from keianhzo, takahirox and netpro2k and removed request for takahirox January 20, 2023 20:02
Base automatically changed from bug/2022-01-20 to master January 20, 2023 20:04
@johnshaughnessy johnshaughnessy force-pushed the feature/bit-pdf branch 2 times, most recently from e0772f4 to ec0545e Compare January 22, 2023 05:48

const hoveredQuery = defineQuery([HoveredRemoteRight]);
export function pdfMenuSystem(world: HubsWorld, sceneIsFrozen: boolean) {
const menu = anyEntityWith(world, PDFMenu)!;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to avoid this call to anyEntityWith.

One option is to memoize menu, as in

let menu;
export function pdfMenuSystem(world: HubsWorld, sceneIsFrozen: boolean) {
  menu = menu || anyEntityWith(world, PDFMenu)!;
  ...

Another option that seems nice is to set menu in a preload, since it feels correct to assign it as part of initialization. This commit explores that idea bbbe398 . The calling code looks like this:

let menu: EntityID;
preload(
  repeatUntilTrue(() => {
    menu = (window.APP && APP.world && anyEntityWith(APP.world, PDFMenu)) as EntityID;
    return !!menu;
  })
);

The inner function will be repeated in a setInterval until it returns true.

I don't know the best way to handle cases like this where we are assuming that some long-lived entity will be created during initialization, and we want to hold a reference to it. I'd like to avoid sticking more things on APP when this comes up.

For now, the simplest solution I could come up with is to just call anyEntityWith every tick (with a ! for type coercion), even though it'll be the same entity every time.

Copy link
Contributor

Choose a reason for hiding this comment

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

For video menu we just made a query and grabbed the first element, which is basically the same as anyEntityWith.

I definitely like the idea of doing it in a preload. The setInterval seems surprising. Ideally we should be able to do it synchronously. Maybe the functions you pass to prelaod get called after APP has been set up and passed a world. Some obviously will also be async after that, but this one shouldn't need to be.

We could also just have a way to get the world with a promise which would remove the need to modify preload.

src/bit-systems/pdf-menu-system.ts Show resolved Hide resolved
src/bit-systems/pdf-system.ts Show resolved Hide resolved
src/inflators/pdf-loader.ts Show resolved Hide resolved
src/bit-systems/pdf-menu-system.ts Outdated Show resolved Hide resolved
src/utils/load-pdf.tsx Outdated Show resolved Hide resolved
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.

None yet

3 participants