Skip to content

Comments

Fix for panic when removing RenetServer or RenetClient resource#8

Merged
Shatur merged 5 commits intosimgine:masterfrom
paul-hansen:fix-reload-resource-not-exist
Apr 9, 2023
Merged

Fix for panic when removing RenetServer or RenetClient resource#8
Shatur merged 5 commits intosimgine:masterfrom
paul-hansen:fix-reload-resource-not-exist

Conversation

@paul-hansen
Copy link
Contributor

@paul-hansen paul-hansen commented Apr 9, 2023

Fixes #7

Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

Thanks! Could you try to put these systems to CoreSet::StateTransition instead and apply .before(run_enter_schedule::<S>)? This change will make the intent obvious and comments won't be necessary.
I also think that you could call .in_base_set on a tuple to avoid calling it on each system.

@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (fa3d2be) 96.69% compared to head (120474f) 96.73%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
+ Coverage   96.69%   96.73%   +0.03%     
==========================================
  Files          13       13              
  Lines         969      979      +10     
==========================================
+ Hits          937      947      +10     
  Misses         32       32              
Impacted Files Coverage Δ
src/client.rs 95.54% <100.00%> (+0.17%) ⬆️
src/server.rs 96.47% <100.00%> (+0.06%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@paul-hansen
Copy link
Contributor Author

Yeah that's much clearer. I hadn't noticed CoreSet::StateTransitions before, learned some things, thanks!

Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

One minor suggestion. And could you also update CHANGELOG.md?

@Shatur Shatur enabled auto-merge (squash) April 9, 2023 20:05
Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

Thank you a lot!

@Shatur
Copy link
Contributor

Shatur commented Apr 9, 2023

Test fails, I will take a look.

@paul-hansen
Copy link
Contributor Author

paul-hansen commented Apr 9, 2023

Hmm also looking, just running cargo test works locally for me.

@Shatur
Copy link
Contributor

Shatur commented Apr 9, 2023

Looks like it happens sometimes.

@paul-hansen
Copy link
Contributor Author

Ah yeah I did get it after a few more tries.

@Shatur
Copy link
Contributor

Shatur commented Apr 9, 2023

Fixed it. It turns out that in tests we had two app.update() after initialization because of the one frame delay. No longer needed!

@Shatur Shatur merged commit 772503a into simgine:master Apr 9, 2023
@paul-hansen
Copy link
Contributor Author

Nice!

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.

Panic if you try to remove RenetClient or RenetServer resources after registering events

3 participants