monitor selection fallback has been added on web build#24208
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨ |
|
The change here is trivial (it adds a |
|
I had approved this PR before the disclaimer was edited into the PR description. That being said, I hear @ChristopherBiscardi’s concern and am inclined to agree but more so because, for this particular issue, I believe a similar or the same solution could have been more easily made by reading the linked issue and looking at the code. The methods used to get to this solution with the use of AI feel excessive. |
If you could explain your approach I could implement it without this flow I have more context now. AI tools were used for context mainly due to my lack of familiarty with the codebase and game engine eco system and being a first time contributor to this codebase. I completely agree that change is trivial but for someone who is a stranger to the codebase the flow encouraged gaining context and fast delivery but not the fix itself. During the early phases of this PR I did not understood the depth of the issue that is how complicated the fix was. The flow was considered to understand that depth in a short time frame. That is to diagnose where the problem lies not the fix itself and how complex the problem is. Regardless if you refer me to your solution I can approach the problem with your intended flow. However, it will yield the same outputs that were obtained in testing. |
So, if I were to approach this issue for the first time, I would have looked at the additional logs provided in the GH issue to see where the source of the panicking was. In the first screenshot in the first few lines, it mentions “[some user specific directory]/crates/bevy_winit/src/system.rs:337”. I would verify whether that is the place in the code where the panicking was, and it appears to be, especially because the unwrap_or_else panic’s statement matches the “Could not find monitor for…” error string. At that point, since Alice mentioned in the PR that the code should definitely not panic, I would have considered writing something similar to this PR, where instead of an unwrap, it returns None and does a My verification would have been different though. I would have relied on the minimal reproduction example to test that the code fixes the issue. (Sometimes I just replace one of the existing examples code with a reproduction example to more quickly spin a reproduction up)
The phrase “fast delivery” gives me pause here. I personally think it’s worth it to slow down and learn more about Bevy in the process of doing an issue, but if this issue is something you urgently need fixing for your own project that depends on Bevy, then I can understand. Otherwise, as someone who’s also learning the codebase and has only had a few months as a more regular contributor, I recommend slowing down to learn a bit more! I feel like this specific issue could teach someone the basics about wasm builds https://github.com/bevyengine/bevy/tree/main/examples#wasm
I understand using AI tools to learn about the codebase in this case, but as someone who also relatively recently started working on Bevy, I would really recommend reviewing other people PR’s, trying to reproduce reported issues / verifying other people’s fixes, and just in general conversing. There’s a lot to learn and many people who you can talk to to figure things out together 😄 |
I think this discussion will become too long for the scope of the issue for now let's just discuss on finalizing this issue What becomes of this PR if you want I can rewrite the fix more minimally and we can end this issue and move onto others. Or to be honest the current fix seems fine and I think we are clear on the part where the AI was used for context mainly. And related automations for testing and other purposes. If you forget for a minute the AI disclaimer doesn't exist I hope you will arrive at the same conclusion or maybe you agreed somewhere on this thread. (I think by approving this PR before I edited the AI disclaimer) Just mention the steps you want me to take I have seen your messages and have made points on them to keep in mind for the next PR |
Objective
fix #23649
Solution
I have added a warning fallback when monitor selection code is trigerred preventing a panic on unrwap
Testing
The testing was done by building a web build then pressing SPACEBAR to trigger relevant logic and logs were monitored
Showcase (Option) A video of testing the functionality
Screencast.from.2026-05-09.17-03-06.webm
AI disclaimer
These files were moved out of the directory
warning that indicates the user of this behavior in developer console