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

Replace internal uses of insert_or_spawn_batch #18035

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

Conversation

JaySpruce
Copy link
Contributor

@JaySpruce JaySpruce commented Feb 25, 2025

Objective

insert_or_spawn_batch is due to be deprecated eventually (#15704), and removing uses internally will make that easier.

Solution

Replaced internal uses of insert_or_spawn_batch with try_insert_batch (non-panicking variant because insert_or_spawn_batch didn't panic).

All of the internal uses are in rendering code. Since retained rendering was meant to get rid non-opaque entity IDs, I assume the code was just using insert_or_spawn_batch because insert_batch didn't exist and not because it actually wanted to spawn something. However, I am not confident in my ability to judge rendering code.

Copy link
Contributor

@Carter0 Carter0 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Can you split apart the "replace internal usages" from the "deprecate"? The former is uncontroversial, but we're definitely going to get users screaming if we deprecate this without a clear answer on what to do for "allocate this specific entity ID".

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Feb 25, 2025
@JaySpruce JaySpruce changed the title Deprecate insert_or_spawn_batch and replace internal uses Replace internal uses of insert_or_spawn_batch Feb 26, 2025
@JaySpruce
Copy link
Contributor Author

I had thought not supporting it was the intention, do you know what the official way to do that should look like?

@alice-i-cecile
Copy link
Member

I had thought not supporting it was the intention, do you know what the official way to do that should look like?

It is! But I've gotten serious pushback from users before when attempting to just remove it: there's clearly a disconnect there. I'm not sure what the right path forward is there, but we can/should definitely stop using this internally for now.

@alice-i-cecile
Copy link
Member

Adding to the 0.16 milestone; since this is showing up in real games as a bizarre performance issue I'd like to get the internals moved over ASAP.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Needs-Testing Testing must be done before this is safe to merge C-Performance A change motivated by improving speed, memory usage or compile times and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 1, 2025
@ElliottjPierce
Copy link
Contributor

Running through some examples on this PR compared to main:

  • parenting example gives error: ERROR bevy_core_pipeline::core_3d::main_opaque_pass_3d_node: Error encountered while rendering the opaque phase InvalidViewQuery This is not present on main. The example still works, so this may be superficial but is worth investegating.
  • alien_cake_addict example has exactly the same issue on launch.
  • mesh_picking example has exactly the same issue on launch.
  • testbed_3d example has exactly the same issue on many of the test scenes.
  • loading_screen example has the same error when transitioning from scene 1 to scene 1, and gives the additinal error ERROR bevy_core_pipeline::core_3d::main_transparent_pass_3d_node: Error encountered while rendering the transparent phase InvalidViewQuery when going from scene 2 to scene 2. Going from scene 1 to 2 or visa versa works fine. It is functional aside from the error message.

These errors are not present on the main branch. I am really unexperienced in all of the rendering side of things, so I don't think I'm the right person to track down the cause here. But I'm happy to test again if anyone has ideas.

I did not notice any visual differences compared to main, and I didn't see entities disappear unexpectedly.

I only spot checked examples that looked suspect to me, so there may be errors I'm missing.

If you think it's worth it, I can add an additional example that just spawns and despawns stuff a bunch to see if that triggers any red flags.

@JaySpruce
Copy link
Contributor Author

These errors are not present on the main branch.

I get those errors on main; I bisected it to 7f14581 (#16782, merged on Feb 24), and it turns out there's already an issue for it: #18094

@ElliottjPierce
Copy link
Contributor

I get those errors on main

It's strange that I didn't but ok. In that case, I think we proceed. I couldn't find anything else fishy. @alice-i-cecile your thoughts when you get the chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Testing Testing must be done before this is safe to merge
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants