search

Trouv / bevy_ecs_ldtk

ECS-friendly ldtk plugin for bevy, leveraging bevy_ecs_tilemap
Other
630 stars 74 forks source link

feat!: upgrade to bevy 0.12 #265

Closed Trouv closed 5 months ago

Trouv commented 8 months ago

Closes #259 Also, I think updating to 0.12 implicitly... Closes #233 Closes #111

This PR upgrades the bevy dependency to 0.12, as well as bevy_ecs_tilemap to 0.12. For dev-dependencies, it also upgrades bevy_rapier2d to 0.23, and bevy-ecs-egui to 0.21. Most other changes were just necessary to get the lib/tests/doc-tests/examples to compile.

This was a little more complicated for ldtk_external_level.rs and ldtk_project.rs since they are updating to use bevy assets v2. However, using assets v2 did help improve these modules slightly. In particular, it was easier to add dependent assets to the load context, and having an associated type for the error allowed me to remove the dependency on anyhow entirely.

Another slightly more difficult update was the process_ldtk_assets system. However, I'm quite happy with the distinction betwen AssetEvent::Added and AssetEvent::LoadedWithDependencies. bevy_ecs_ldtk now waits for the latter before spawning levels, which is functionally what it meant to do originally.

I've verified that all the examples are visually OK, and also checked out some visual edge cases. Any visual bugs I noticed aren't new.

Will keep this as a draft until bevy_ecs_tilemap 0.12 releases.

Twyker commented 7 months ago

any chance to make this work with the most current PR branch for the 0.12 fixes on bevy_ecs_tilemap side? tried it like this but sadly didn't work :x image

Trouv commented 7 months ago

Try depending on bevy_ecs_ldtk version 0.8, and using these branches as a patch rather than as deps:

[patch.crates-io]
bevy_ecs_ldtk = { git = "https://github.com/trouv/bevy_ecs_ldtk", branch = "feat/bevy-0.12" }
bevy_ecs_tilemap = { git = "https://github.com/divark/bevy_ecs_tilemap", branch = "0.12-fixes" }
Trouv commented 7 months ago

Also - for those looking to upgrade early, be aware that bevy_ecs_ldtk has had a lot of breaking changes. There's a migration guide for this - it renders better in Mdbook than in github but here it is: https://github.com/Trouv/bevy_ecs_ldtk/blob/5feb5cc3452e0b7898db8348e0ebcea890c63e15/book/src/how-to-guides/migration-guides/migrate-from-0.8-to-0.9.md

musjj commented 7 months ago

I'm getting a crash when using a RenderTarget on the camera. It worked just fine when I was at 0.11.

thread '<unnamed>' panicked at $HOME/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wgpu-0.17.2/src/backend/direct.rs:3056:5:
wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 4, Vulkan)>`
    In a set_bind_group command
      note: bind group = `tilemap_view_bind_group`
    Dynamic binding offset index 0 with offset 2304 would overrun the buffer bound to bind group 0 -> binding 0. Buffer size is 1536 bytes, the binding binds bytes 0..608, meaning the maximum the binding can be offset is 928 bytes

Any clue why this is happening?

Trouv commented 7 months ago

I'm not sure, does it happen when using a RenderTarget w/ bevy_ecs_tilemap alone?

musjj commented 7 months ago

It's a really bizzare issue. I made a reproduction crate here: https://github.com/musjj/tilemap_bug

The crash is basically caused by this snippet:


fn add_camera(
    camera: Query<&CanvasCamera>,
    mut commands: Commands,
    level_query: Query<&LevelIid>,
    canvas_query: Query<&Handle<Image>, With<Canvas>>,
) {
    if let Ok(iid) = level_query.get_single() {
        if camera.is_empty() {
            let canvas = canvas_query.single();
            commands.spawn((
                Camera2dBundle {
                    camera: Camera {
                        order: -1,
                        target: RenderTarget::Image(canvas.clone()),
                        ..default()
                    },
                    ..default()
                },
                CanvasCamera,
            ));
        }
    }
}

Changing level_query: Query<&LevelIid> to level_query: Query<&FooBar> (or any other queries) stops it from crashing. So I'm guessing that holding a reference to LevelIid is causing the issue here. But I don't understand why. Maybe some unsafe shenanigans?

Can you try to reproduce it?

musjj commented 7 months ago

Another thing: it's not the &LevelIid query itself that is causing the crash. You can put the camera spawn logic outside of the scope of &LevelIid and it will not crash. This is some crazy black magic stuff...

Trouv commented 7 months ago

Your project crashes for me too. Let me try to see if it happens w/ just bevy_ecs_tilemap

Trouv commented 7 months ago

Ok - after a lot of testing, I believe this is caused by spawning the camera w/ the custom render target late. I was able to recreate this on a bevy_ecs_tilemap not by querying for their components, but by waiting until frame 10 to spawn the camera: 

fn add_camera(
    camera: Query<&CanvasCamera>,
    mut commands: Commands,
    mut frame_count: Local<usize>,
    canvas_query: Query<&Handle<Image>, With<Canvas>>,
) {
    if *frame_count > 10 {
        if camera.is_empty() {
            let canvas = canvas_query.single();
            println!("spawning rendertarget camera");
            commands.spawn((
                Camera2dBundle {
                    camera: Camera {
                        order: -1,
                        target: RenderTarget::Image(canvas.clone()),
                        ..default()
                    },
                    ..default()
                },
                CanvasCamera,
            ));
        }
    } else {
        *frame_count += 1;
    }
}

I'll comment on the bevy_ecs_tilemap PR. Thanks for bringing this to my attention.

cscorley commented 7 months ago

Thanks for all your hard work on this plugin!

Got to test this out, and for the most part, everything works! Only thing I noticed that was different is the level layers higher than my main sprites. My solution to this was to set the LdtkWorldBundle to have a transform Transform::from_xyz(0., 0., -0.1)

my current lock for these packages:

[[package]]
name = "bevy_ecs_ldtk"
version = "0.8.0"
source = "git+https://github.com/trouv/bevy_ecs_ldtk?branch=feat/bevy-0.12#5feb5cc3452e0b7898db8348e0ebcea890c63e15"
dependencies = [
 "bevy",
 "bevy_ecs_ldtk_macros",
 "bevy_ecs_tilemap",
 "derive-getters",
 "derive_more",
 "hex",
 "paste",
 "path-clean",
 "regex",
 "serde",
 "serde_json",
 "thiserror",
]

[[package]]
name = "bevy_ecs_ldtk_macros"
version = "0.8.0"
source = "git+https://github.com/trouv/bevy_ecs_ldtk?branch=feat/bevy-0.12#5feb5cc3452e0b7898db8348e0ebcea890c63e15"
dependencies = [
 "proc-macro2",
 "quote",
 "syn 1.0.109",
]

[[package]]
name = "bevy_ecs_tilemap"
version = "0.12.0"
source = "git+https://github.com/divark/bevy_ecs_tilemap?branch=0.12-fixes#50718d6ec60ff29abc0496446a4a1614d1a779fe"
dependencies = [
 "bevy",
 "log",
 "regex",
]
Trouv commented 7 months ago

FYI @musjj a fix has been merged to the bevy_ecs_tilemap PR that at least stops the crash. It looks like they're running into other issues as a result though.

Trouv commented 7 months ago

Got to test this out, and for the most part, everything works! Only thing I noticed that was different is the level layers higher than my main sprites. My solution to this was to set the LdtkWorldBundle to have a transform Transform::from_xyz(0., 0., -0.1)

Huh. I just tested on the new commit they pushed, and it sounds similar to the new bugs: https://github.com/StarArawn/bevy_ecs_tilemap/pull/489#issuecomment-1826925558

However, your lockfile suggests you're using the previous commit, not the new one.

How are you spawning your camera?

Trouv commented 7 months ago

FYI all - the migration guide is now rendered here: https://trouv.github.io/bevy_ecs_ldtk/how-to-guides/migration-guides/migrate-from-0.8-to-0.9.html

cscorley commented 7 months ago

Got to test this out, and for the most part, everything works! Only thing I noticed that was different is the level layers higher than my main sprites. My solution to this was to set the LdtkWorldBundle to have a transform Transform::from_xyz(0., 0., -0.1)

Huh. I just tested on the new commit they pushed, and it sounds similar to the new bugs: StarArawn/bevy_ecs_tilemap#489 (comment)

However, your lockfile suggests you're using the previous commit, not the new one.

How are you spawning your camera?

I just tested this again (lockfile to follow) and still have the same issue.

The camera spawns with Camera2dBundle::default() only. I also have this system to make sure the whole level fits the window at all times. However, even with this system disabled, things are off. I'm honestly fine with the -0.1, but wanted it to be known.

It definitely seems like the new bug (https://github.com/StarArawn/bevy_ecs_tilemap/pull/489#issuecomment-1826925558).

pub fn fit_level(
    mut camera_query: Query<(&mut OrthographicProjection, &mut Transform)>,
    current_level: Option<Res<CurrentLevel>>,
) {
    if current_level.is_none() {
        return;
    }

    let (mut orthographic_projection, mut camera_transform) = camera_query.single_mut();
    let current_level = current_level.unwrap();

    let width = current_level.pixel_dimensions.x;
    let height = current_level.pixel_dimensions.y;

    camera_transform.translation.x = width / 2.;
    camera_transform.translation.y = height / 2.;

    orthographic_projection.scaling_mode = bevy::render::camera::ScalingMode::AutoMin {
        min_width: width,
        min_height: height,
    };
    orthographic_projection.area = Rect::new(0., 0., width, height);
}

image

[[package]]
name = "bevy_ecs_ldtk"
version = "0.8.0"
source = "git+https://github.com/trouv/bevy_ecs_ldtk?branch=feat/bevy-0.12#5feb5cc3452e0b7898db8348e0ebcea890c63e15"
dependencies = [
 "bevy",
 "bevy_ecs_ldtk_macros",
 "bevy_ecs_tilemap",
 "derive-getters",
 "derive_more",
 "hex",
 "paste",
 "path-clean",
 "regex",
 "serde",
 "serde_json",
 "thiserror",
]

[[package]]
name = "bevy_ecs_ldtk_macros"
version = "0.8.0"
source = "git+https://github.com/trouv/bevy_ecs_ldtk?branch=feat/bevy-0.12#5feb5cc3452e0b7898db8348e0ebcea890c63e15"
dependencies = [
 "proc-macro2",
 "quote",
 "syn 1.0.109",
]

[[package]]
name = "bevy_ecs_tilemap"
version = "0.12.0"
source = "git+https://github.com/divark/bevy_ecs_tilemap?branch=0.12-fixes#dbc6b17365434bd0194d82a5426ce0561829e628"
dependencies = [
 "bevy",
 "log",
 "regex",
]
Trouv commented 7 months ago

Made a post about using this for bevy jam 4, it reiterates the patch you need to use: https://github.com/Trouv/bevy_ecs_ldtk/discussions/273#discussion-5916548

Trouv commented 7 months ago

New migration guide link (same one, it just moved): https://trouv.github.io/bevy_ecs_ldtk/latest/how-to-guides/migration-guides/migrate-from-0.8-to-0.9.html

MScottMcBee commented 7 months ago

Just tested, #232 still exists on this branch. I'll look into it and update the ticket when I know more

Trouv commented 5 months ago

FYI, just pulled some changes into this branch for updating to LDtk 1.5.3. Support for previous versions is being dropped

Trouv commented 5 months ago

I've decided to merge this into main to try and simplify this repo a bit. So, users may need to change their patches accordingly