Weasy666 / bevy_svg

A simple and incomplete SVG drawer for the Bevy engine.
Apache License 2.0
108 stars 26 forks source link

Look into using AssetLoader #2

Closed Weasy666 closed 2 years ago

Weasy666 commented 3 years ago

Change the file loading from direct loading per File::open() to loading by Bevy's AssetLoader.

robert-chiniquy commented 3 years ago

Just a thought, hope it's welcome from a stranger, an easy and useful first step towards this might be to expose a constructor from bytes rather than from a file.

Weasy666 commented 3 years ago

You are welcome! I haven't had time to take a closer look at Bevy's AssetLoader. Are you somewhat familiar with it? Well...i guess...creating/loading an SVG from bytes could be useful either way, so i will definitely look to implement that.

robert-chiniquy commented 3 years ago

I haven't done it myself, but looking at the simplest example I know: https://github.com/jamadazi/bevy_asset_ron/blob/master/src/lib.rs#L28 makes it look like bytes are the interface. And like you said, I thought it would be useful either way.

Weasy666 commented 3 years ago

I have implemented from_bytes and from_reader functions to SvgBuilder and will take a stab at the asset loader the next view days, as this is a good diversion after hitting a wall while trying to fix issue #3 😫

mdevlamynck commented 3 years ago

If you want to take a look I've implemented an AssetLoader for Svg with an associated Mesh in https://github.com/mdevlamynck/bevy_svg/tree/asset_loader. I forked from the bevy-0.5 branch and refactored the code quite a bit (for instance I removed the Origin type and put the origin in the center of the svg during the construction of the paths) so a PR might be to many changes.

Weasy666 commented 3 years ago

Oh...wow! That really are a load of changes. I will look through it on the weekend and come back to you. Thank you!

mdevlamynck commented 3 years ago

Also I might have a clue for #3. I'm not sure yet (I think the abs_transform() is not used when reading usvg tree).

mdevlamynck commented 3 years ago

Just fixed #3 in my fork. I'll try to extract the fix into a PR, do you prefer I target bevy-0.5 or main?

bsurmanski commented 2 years ago

I was looking into this project (hoping to use it). @mdevlamynck, I took a look at the asset-loader branch.

Here's my unsolicited (and hopefully helpful) feedback: 1) you removed the 'prelude' module. For bevy, at least, it seems conventional have one. 2) Instead of using SvgBundleConfig, it seems more conventional to implement Default. See PbrBundle, for example. You'd do:

.insert_bundle(PbrBundle {
                    mesh: asset_server.load("mymesh.gltf#Mesh0/Primitive0"),
                    material: asset_server.load("mymesh.gltf#Material0"),
                    ..Default::default()
                })

3) The "NeedMeshUpdate" component seems a little hacky. Consider: remove "mesh" from the SvgBundle. In the attach_mesh system, query for Without<Handle> instead, and attach the mesh there. (I could be wrong on this one though, idk) 4) In the readme, it mentions support for the 'svgz' extension. Add that to the 'extensions' list in AssetLoader, or update the readme. 5) the readme.md example hasn't been updated 6) you added a few editor files that probably shouldn't be there, or should be a seperate PR (flake.nix, rustfmt.toml(?))

I skipped over a lot of the heavy code, but hopefully that's helpful for getting a PR through.

Weasy666 commented 2 years ago

Thanks for taking a look. I am in the process of updating the crate to the new Bevy renderer and while being at it, i am implementing AssetLoader too. So...it should be in the next 0.* release, which is (hopefully) ready when bevy 0.6 will be released.

mdevlamynck commented 2 years ago

@bsurmanski

  1. If it's the norm in the community I agree :)
  2. SvgBundleConfig contains fields not in SvgBundle so I'm not sure it would work (and SvgBundleConfig implements Default). But I agree it would be nice to only use SvgBundle.
  3. IIRC it was crashing without a mesh. I'd need to try your solution to see if it works :)
  4. Need to try that too to see if it works :)
  5. Agreed, needs to be updated :)
  6. Yes, I'd need to clean the branch, it contains both things for a PR and for my personal use ^^'

In the end I changed the crate too much to be a realistic candidate for a PR. But at least it can be an example.

Weasy666 commented 2 years ago

Ok...i am somewhat stuck and needed a short break from the new renderer, so i've decided to implement AssetLoader and backported it to bevy-0.5, so you can test with both branches main and bevy-0.5. I will test it for a few days and if i don't find any serious problems, i will release a breaking change at the end of next week.

bsurmanski commented 2 years ago

@mdevlamynck,

Regarding (2), looks like SvgBundleConfig is the following:

pub struct SvgBundleConfig {
    /// SVG to render.
    pub svg:      Handle<Svg>,
    /// Position at which the [`SvgBundle`] will be spawned in Bevy.
    /// The center of the SVG will be at this position.will be at this position.
    pub position: Vec3,
    /// Value by which the SVG will be scaled, default is (1.0, 1.0).
    pub scale:    Vec2,
}

Looks like svg is in the SvgBundle, and position and scale are represented in the Transform, right?

Regarding (3), there might be other components that would need a deferred insertion too, like Draw or MainPass? Idk.

Weasy666 commented 2 years ago

AssetLoader is now available in the newly released version 0.4 for Bevy 0.5. Updating to the new Bevy renderer (in Bevy 0.6) will still take a little longer.