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

Send SceneInstanceReady only once per scene #11002

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

daxpedda
Copy link
Contributor

Objective

Send SceneInstanceReady only once per scene.

Solution

I assume that this was not intentional.
So I just changed it to only be sent once per scene.


Changelog

Fixed

  • Fixed SceneInstanceReady being emitted for every Entity in a scene.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@mockersf
Copy link
Member

Did you notice receiving several times this event?

It should already be sent only once

@daxpedda
Copy link
Contributor Author

daxpedda commented Dec 16, 2023

Yes, I received it multiple times, exactly as many times actually as I had entities in the scene.

I'm not familiar with Bevy's codebase, but from how I understand the code it seems obvious that it calls send_event() once per entity.

EDIT: I only tested in v0.12.1 actually, should I test it on main as well?

@mockersf
Copy link
Member

mockersf commented Dec 16, 2023

I'm on main and seeing this event only once, but this code shouldn't have changed since the 0.12.1

Could you share the scene you're using? I tried with a few gltf files.

Your change should be OK, just trying to understand why you're having this issue here

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Scenes Serialized ECS data stored on the disk labels Dec 17, 2023
@daxpedda daxpedda force-pushed the scene-instance-ready branch from 6387ea5 to 23afd0e Compare December 17, 2023 10:26
@daxpedda
Copy link
Contributor Author

Could you share the scene you're using? I tried with a few gltf files.

I added a unit test. Without the change in this PR the unit test will fail.

@daxpedda daxpedda force-pushed the scene-instance-ready branch 2 times, most recently from 97fd191 to 8aa4b88 Compare December 17, 2023 10:33
@daxpedda daxpedda force-pushed the scene-instance-ready branch 2 times, most recently from fc3c346 to ad6fd09 Compare December 18, 2023 08:41
@daxpedda
Copy link
Contributor Author

Rebased on main after #11003.

@mockersf
Copy link
Member

nice test, thanks!

unlike scenes from gltfs, yours has two root entities, which explains what you're seeing 👍

@daxpedda daxpedda force-pushed the scene-instance-ready branch from ad6fd09 to b19f4ea Compare January 6, 2024 23:27
@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 6, 2024

Rebased after cfcb688.

Copy link
Contributor

@jdm jdm left a comment

Choose a reason for hiding this comment

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

I appreciate the unit test for this change!

@rparrett rparrett added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 30, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 5, 2024
Merged via the queue into bevyengine:main with commit 2fd5d46 Feb 5, 2024
23 checks passed
lynn-lumen pushed a commit to lynn-lumen/bevy that referenced this pull request Feb 5, 2024
# Objective

Send `SceneInstanceReady` only once per scene.

## Solution

I assume that this was not intentional.
So I just changed it to only be sent once per scene.

---

## Changelog

### Fixed
- Fixed `SceneInstanceReady` being emitted for every `Entity` in a
scene.
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

Send `SceneInstanceReady` only once per scene.

## Solution

I assume that this was not intentional.
So I just changed it to only be sent once per scene.

---

## Changelog

### Fixed
- Fixed `SceneInstanceReady` being emitted for every `Entity` in a
scene.
github-merge-queue bot pushed a commit that referenced this pull request Jul 6, 2024
# Objective

- Emit an event regardless of scene type (`Scene` and `DynamicScene`).
- Also send the `InstanceId` along.

Follow-up to #11002.
Fixes #2218.

## Solution

- Send `SceneInstanceReady` regardless of scene type.
- Make `SceneInstanceReady::parent` `Option`al.
- Add `SceneInstanceReady::id`.

---

## Changelog

### Changed

- `SceneInstanceReady` is now sent for `Scene` as well.
`SceneInstanceReady::parent` is an `Option` and
`SceneInstanceReady::id`, an `InstanceId`, is added to identify the
corresponding `Scene`.

## Migration Guide

- `SceneInstanceReady { parent: Entity }` is now `SceneInstanceReady {
id: InstanceId, parent: Option<Entity> }`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants