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

Example overhaul #4

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

DasLixou
Copy link
Contributor

@DasLixou DasLixou commented Apr 7, 2024

This completely removes all current examples, as they seem to just be copied from the main vello repository.
Instead, there are two new crates to find inside the examples directory: tiger and util.
util is there to hide all the work of drawing a vello scene with wgpu inside a winit window and is called by tiger.
tiger is a simple 30-liner just loading the Ghostscript_Tiger.svg svg file, loading it and rendering it to a vello scene, which then gets passed to the util::display function.
The util::display function does nothing fancy and the window just keeps the size of the original svg without the possibility of resizing (This can be improved later on if necessary).
Running the example then shows this:
image
It does take a short time because I also removed the multithreading code which can be found in the original vello example because I want to keep it a simple example to just understand the api.

@xStrom
Copy link
Member

xStrom commented Apr 7, 2024

I think disconnecting the Vello re-export question from this PR makes sense. Given that it is a simple change, I guess you can skip having an issue and just open a PR with that change where that discussion can take place.

@DasLixou
Copy link
Contributor Author

DasLixou commented Apr 7, 2024

Ok also had to remove android-stable-check CI - have I done it correctly?

@simbleau
Copy link
Member

simbleau commented Apr 7, 2024

I have so many mixed feelings about this.

Basic thoughts:

  • I like how the util example feels simpler. You did good refactoring that. However, I think util is a bad name and I would prefer winit_demo_runtime or something more descriptive.
  • I wish the effort was put into extracting Vello's with_winit example runtime into an importable crate, similar to what you've done here... That way we could simply import and replace all of the boilerplate here (and with velato) with that demo runtime.
  • I'd prefer the update to usvg was a separate PR.
  • Why was run_wasm removed? Thewith_winit example should work on WASM and that is something I intend to support
  • Why were downloads removed? That is something I intend to keep.
  • Although I don't care as much about Android as a target, @DJMcNab might. I'd wait for his thoughts before touching any Android-related CI.

@DasLixou
Copy link
Contributor Author

DasLixou commented Apr 7, 2024

Using the display in other vello "supercrates" sure is a good idea. And yep I'll undo and move the usvg update to another PR.

The reason why I removed download and wasm stuff was that I feel like the example should only show how to use vello_svg, not vello. Having to maintain more extra code in the examples for wasm support when it's just to showcase the crate is something I wouldn't want personally, so thats the intention behind it

@DJMcNab
Copy link
Contributor

DJMcNab commented Apr 8, 2024

I wish the effort was put into extracting Vello's with_winit example runtime into an importable crate, similar to what you've done here... That way we could simply import and replace all of the boilerplate here (and with velato) with that demo runtime.

This is remarkably messy. I think this is a good impulse, but I don't want to end up in a situation where this integration is used by our consumers, at least with the current state. That is, the name needs to be something like vello_development_runtime, and it should also e.g. print a warning message to console.

However, I also think that it would be a viable path forward to create the vidi crate, which is the integration between winit and Vello. We would in that case expect (e.g.) Xilem to use that directly, so that we can maintain the idiomatic connection in a single place, including with correct integration of frame pacing, etc.


I am also confused about why the Android check was removed. We *need* to at least confirm that the `vello_svg` crate compiles for Android, even if we don't check that the example compiles for Android. But AIUI, it's much easier to perform that check using `cargo apk`, because that sets the correct `CC` environment variable (for example).

@DasLixou
Copy link
Contributor Author

DasLixou commented Apr 8, 2024

We don't need to push a crate for the util - could just keep it as a git dependency.

Ok, I'll readd the android stuff when I'm back at a computer

@DJMcNab
Copy link
Contributor

DJMcNab commented Apr 8, 2024

Keeping it as a git dependency might not be viable, unfortunately

Because the version in git needs to depend on Vello from crates.io, to be usable in these repositories. But it needs to depend on Vello from the local path to be useful in the vello repository

@DasLixou
Copy link
Contributor Author

DasLixou commented Apr 8, 2024

🥲 can someone tell me what I've done wrong in the CI?

@simbleau
Copy link
Member

simbleau commented Apr 8, 2024

The reason why I removed download and wasm stuff was that I feel like the example should only show how to use vello_svg, not vello. Having to maintain more extra code in the examples for wasm support when it's just to showcase the crate is something I wouldn't want personally, so thats the intention behind it

The run_wasm crate is more than just a showcase. It's a litmus test "can I render the tiger on web?" and a show of support to that target. We don't have robust testing for web, so this is the best we have in lou of that. I appreciate your intention to reduce overhead, but this is something I'm not okay removing. In the future I suggest spinning up an issue and asking for concurrence before making large, broad-spanning PRs. As it stands, I won't approve this PR, because I care a lot about compiling to wasm as a downstream user of this myself (in bevy_vello).

To summarize, I appreciate your effort here greatly and your intentions. I love the energy and passion and you're gaining very useful experience in the codebase. I just don't think this is the right direction.

I wish the effort was put into extracting Vello's with_winit example runtime into an importable crate, similar to what you've done here... That way we could simply import and replace all of the boilerplate here (and with velato) with that demo runtime.

This is remarkably messy. I think this is a good impulse, but I don't want to end up in a situation where this integration is used by our consumers, at least with the current state. That is, the name needs to be something like vello_development_runtime, and it should also e.g. print a warning message to console.

However, I also think that it would be a viable path forward to create the vidi crate, which is the integration between winit and Vello. We would in that case expect (e.g.) Xilem to use that directly, so that we can maintain the idiomatic connection in a single place, including with correct integration of frame pacing, etc.
I am also confused about why the Android check was removed. We need to at least confirm that the vello_svg crate compiles for Android, even if we don't check that the example compiles for Android. But AIUI, it's much easier to perform that check using cargo apk, because that sets the correct CC environment variable (for example).

I like the idea of the vidi crate. Is there a larger discussion on this on Zulip? The main concern I have is the translation to bevy. Will there be any dependence on winit features not handled by Bevy? If so, I'll be an advocate for a bevy feature in vidi to make integration easy, or I'll need to reproduce that logic in bevy_vello.

@DJMcNab
Copy link
Contributor

DJMcNab commented Apr 8, 2024

My proposal was originally laid out in #glazier>Vidi.

I'm really confused by your comment on Bevy. Would you please explain what caused you to believe that Vidi would depend on bevy/at all impact an integration between Bevy and Vello?

@DasLixou
Copy link
Contributor Author

DasLixou commented Apr 8, 2024

So I readded all the old examples and moved the usvg dep update to #6 - So this now provides a simple overview/example of the API while keeping the other for testing

@simbleau
Copy link
Member

My proposal was originally laid out in #glazier>Vidi.

I'm really confused by your comment on Bevy. Would you please explain what caused you to believe that Vidi would depend on bevy/at all impact an integration between Bevy and Vello?

Sounds like a non-issue now, I understand the idea here.

At first I thought vidi would be a requirement for vello, to execute frame pacing, etc. correctly, and I was only wondering if that would cause some complexity on the bevy side.

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

4 participants