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

Update to Bevy 0.15 #217

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

Conversation

wiggleforlife
Copy link

@wiggleforlife wiggleforlife commented Dec 2, 2024

Potentially not the best code ever. I didn't update the demo as it depends on https://github.com/FraserLee/bevy_sprite3d, which hasn't been updated. crates/bevy_plugin/tests/test_strings_tables.rs:142 fails, but I'm not really sure what is happening in the test in the first place, so some assistance would be nice :).

@wiggleforlife wiggleforlife marked this pull request as ready for review December 2, 2024 04:42
@Testare
Copy link

Testare commented Dec 4, 2024

FWIW: I don't use a lot of the fancy Yarn features, but I patched my game to use this fork for the 0.15 upgrade and it seems to be working perfectly.

@janhohenheim
Copy link
Member

Thanks a bunch! Will review this proper and when I'm a bit less swamped :)

@wiggleforlife
Copy link
Author

wiggleforlife commented Dec 4, 2024

It looks like 3 of the 4 CI runs have failed because of the mismatched versions. Should we hold the PR until the demo can be updated, or is that ok?

Edit: someone in the Discord told me that Sprite3D has been updated, so I'll work on that

Copy link
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

Looks good to me, just left some nits :)

crates/example_dialogue_view/src/option_selection.rs Outdated Show resolved Hide resolved
crates/example_dialogue_view/src/setup.rs Show resolved Hide resolved
crates/example_dialogue_view/src/setup.rs Outdated Show resolved Hide resolved
crates/example_dialogue_view/src/setup.rs Outdated Show resolved Hide resolved
crates/example_dialogue_view/src/setup.rs Outdated Show resolved Hide resolved
@TimJentzsch
Copy link
Collaborator

There are still some CI failures, one due to Clippy lints added in Rust 1.83, one due to unused variables/imports and some test failures.

@wiggleforlife
Copy link
Author

wiggleforlife commented Dec 7, 2024

some test failures.

As I mentioned in my first comment, I don't understand that test. Do you have any pointers on this?
Debugging the test that fails, I see that original_strings_file_line_ids.len() and strings_file_line_ids.len() both return 11, but the assert adds the forme rand string_table.len() - is string_table.len() supposed to be 0 instead of 24?

@TimJentzsch
Copy link
Collaborator

some test failures.

As I mentioned in my first comment, I don't understand that test. Do you have any pointers on this?
Debugging the test that fails, I see that original_strings_file_line_ids.len() and strings_file_line_ids.len() both return 11, but the assert adds the forme rand string_table.len() - is string_table.len() supposed to be 0 instead of 24?

To be honest, I'm not really familiar with the tests myself.
Maybe @janhohenheim will know more!

@janhohenheim
Copy link
Member

Nearly all tests are taken from the C# / Unity implementation FWIW. I'll look closer into this PR in 2 weeks and give more guidance then. Until then, it might be useful to look at the C# code as a reference :)

@tigerplush
Copy link

I added your branch to my project with
bevy_yarnspinner = { git = "https://github.com/wiggleforlife/YarnSpinner-Rust.git" } in my Cargo.toml.

When adding the plugin with

        .add_plugins(YarnSpinnerPlugin::with_yarn_source(YarnFileSource::folder(
            "events/dialogues",
        )))

I have about 13 FPS.
Uncommenting the plugin insertion, I go up to 60 fps.

Using YarnSpinnerPlugin::new() yields the fps drop too.
I don't do anything with the plugin, though 🤔

I don't have ExampleYarnSpinnerDialogueViewPlugin::new() added because I want to write my own

@tigerplush
Copy link

I tested the yarnspinner demo project from @wiggleforlife s branch: I have about 21FPS on there.
Comparing with the current main of YarnSpinner-Rust where I have a steady 30 FPS - so something is definitely going on in that migration

@TimJentzsch
Copy link
Collaborator

TimJentzsch commented Dec 11, 2024

I tested the yarnspinner demo project from @wiggleforlife s branch: I have about 21FPS on there.
Comparing with the current main of YarnSpinner-Rust where I have a steady 30 FPS - so something is definitely going on in that migration

In the diff of this PR I don't see any obvious thing which could impact performance like this...
So it might even be a regression in Bevy itself.

We might need to do a deeper analysis to track down what's causing these issues.
Unfortunately, I'm quite busy the next couple of weeks so I probably can't help much for now

@tigerplush
Copy link

I tried 4 hours creating a flamegraph and doing traces, to no avail.
I don't understand where it's coming from (or even how to measure performance in a definitive way)

I doubt that bevy is the issue, I don't think this crate would be the first to notice it and there are no open issues regarding performance, I think it more likely that something was missed during migration

@wiggleforlife
Copy link
Author

wiggleforlife commented Dec 13, 2024 via email

@tigerplush
Copy link

I should state that Framerate drops only happen in develop, not in release profile 🤔

@wiggleforlife
Copy link
Author

wiggleforlife commented Dec 14, 2024

OK, I have a few observations from using iyes_perf_ui on the demo in 0.14 and 0.15. I have a Ryzen 7 7840HS + Radeon 780M iGPU

  • 0.15 averages 30fps, 0.14 averages 23fps, but it seems like 0.14's framerate might increase after a while
  • 0.15's entity count increases throughout the demo, ending in 1095 when Clippy and Ferris shout "WHAZZUP," while 0.14 maintains a sub-200 entity count, ending with 187 entities
  • They both have flickering shaders, but it's worse in 0.15 (safe to say that this isn't an issue with YS)
  • 0.15 has bigger text sizing... I bet this is just a simple mistake somewhere in the text styles (on second thought, iyes_perf_ui's overlay is bigger too... I wonder if this is some DPI update in Bevy?)
    image

It's possible that instead of replacing text entities, I made it so that it just adds new ones. I'll have a look tomorrow, as it's 23:30 here :P

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.

6 participants