Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

Notification system #23

Open
setzer22 opened this issue Mar 23, 2022 · 12 comments
Open

Notification system #23

setzer22 opened this issue Mar 23, 2022 · 12 comments
Labels
good first issue Good for newcomers

Comments

@setzer22
Copy link
Owner

Sometimes it is necessary to give a message to the user when something goes wrong (or succeeds). A good example is the "Export OBJ" node, which currently doesn't do any indication of either success or failure.

In most apps this is done by a notification system that reports messages in bubbles. The bubbles stay alive for a certain time and show the message. The user should be able to click and dismiss the bubble.

A good example of what is needed can be seen here:
image

This could also become a standalone helper crate for other egui apps

@setzer22 setzer22 added the good first issue Good for newcomers label Mar 23, 2022
@inact1v1ty
Copy link
Contributor

Hi @setzer22! I've been following this project for a while. In fact, shortly before this repository appeared, I myself was thinking of doing something like this (nodes + rust + integration into engines). 🙂
I would like to tackle this issue (always wanted to mess up with egui).

@CatCode79
Copy link

There are already a couple of crates that would do for us:
https://crates.io/crates/egui-toast
https://crates.io/crates/egui-notify
I don't know them well though and I can't tell you the pros and cons.

@inact1v1ty
Copy link
Contributor

Oh, that prooved to be mush easier than I thought, thanks @CatCode79 🙂

@setzer22
Copy link
Owner Author

setzer22 commented Sep 15, 2022

Hi @inact1v1ty! 🙂 I'm happy to know people are following the project, it's always nice to see people show up!

Sorry it took me a while to reply. Currently busy with lots of stuff, and doing some very minimal blackjack work in the background.

Integrating egui-toast or egui-notify sounds like a great idea. Is this something you'd like to tackle? I could give you a few pointers:

  • This is code that would need to live inside the blackjack_ui crate.
  • In particular, I think the RootViewport struct defined in application.rs would be a good spot to store this.
  • In its update method, there is a TODO (line 236) that should show a notification when there's an error. The current behavior where it panics is not nice. I want to avoid panics in blackjack as much as possible because they're very disruptive for the user (total crash, loose unsaved work).
  • The AppRootAction enum could get an extra variant ShowNotification so that the app_context can also send notifications by pushing this action to the list.
  • On application_context.rs, line ~70, you will see there's an eprintln call. This error should be shown in a notification toast, not just printed to stderr.

There are other things related to error reporting that can be made nicer. But this should be a good starting point 😄

@inact1v1ty
Copy link
Contributor

Hello again @setzer22! Sadly I was not able to start working on the issue before due to personal and world-country-situation reasons. Now I'm safe and sound and have time and energy, so I'm able to continue contributing to open source and your amazing project in particular! Thank you for your tips, I'll come back later with some code done 🙂

@setzer22
Copy link
Owner Author

Glad to hear everything's better now 🙂 Looking forward to your changes! Let me know if you find any issues.

@inact1v1ty
Copy link
Contributor

Hi @setzer22! Both libraries are using egui 0.19, so I decided to try to bump the version in blackjack and contribute it as a pr also. I have bumped egui, glam, rend3, wgpu, winit, and egui_node_editor and etc and have successfully fixed almost all compilation errors. I have one question though, the not-forked version of egui-wgpu doesn't have zoom_level and resolution arguments in RenderPass::execute_with_renderpass method. Is there a way to work around that that you may now?

@inact1v1ty
Copy link
Contributor

inact1v1ty commented Oct 1, 2022

Okay, there are one more such thing in egui_winit 😞 The WindowOrSize type in take_egui_input. I think I need some help from you: should I try to work around that and how, or should I bump version in your fork and use it. Maybe if your changes fix some important things that can't be worked around without them I should pr these changes to upstream egui crates 🤔

For both your changes I can make code compile without them, but as I understand they were made to mitigate some erroneous runtime behaviour, and I don't know exactly what behaviour and how.

@inact1v1ty
Copy link
Contributor

Just to clarify - in the PR with egui-0.19 I was able to work around the API changes: both RenderPass::execute_with_renderpass signature and lack of WindowOrSize. So, waiting for your review 🙂

@setzer22
Copy link
Owner Author

setzer22 commented Oct 3, 2022

the not-forked version of egui-wgpu doesn't have zoom_level and resolution arguments in RenderPass::execute_with_renderpass method. Is there a way to work around that that you may now?

I introduced these changes back when this code lived in egui_wgpu_backend around egui 0.15, and at the time this was necessary to get the right zooming behavior. But everything seems to work without it now? So maybe there has been some change that fixed the original issue I wasn't aware of 🤔 I need to investigate a bit further but as long as it works, the less forks we need the better!

@inact1v1ty
Copy link
Contributor

Well, got back to working on this and I have following thoughts:

Also, both these libraries are not so complicated.
And one more thing - blackjack for a lot things spills out info every frame, so we have both to track already spawned toasts and the library tracks toasts internally.

So, I think that for a program targeting to become a day-to-day tool will benefit from custom notification solution with buttons for interaction, animations and etc.
This is my plan for now - to implement custom notifications for blackjack and maybe upstream some features from it to those two libraries.

@setzer22
Copy link
Owner Author

And one more thing - blackjack for a lot things spills out info every frame

The plan is to not necessarily recompute everything every frame. Specifically, I don't think the node graph should run and the mesh data get recomputed every frame at 60 frames per second. This is just how things are right now because I haven't gotten around to implement caching or change detection.

This is my plan for now - to implement custom notifications for blackjack and maybe upstream some features from it to those two libraries.

Indeed, with big software like this sometimes it makes more sense to bring in dependencies into the codebase so we can have better control and provide a more custom user experience.

If integrating requires substantial changes, I think it's fine to just bring the code into blackjack_ui. Rust can be a bit limiting when interacting with external dependencies with everything being private by default and the orphan rules. Just remember to check the licenses and always credit the original authors if you take code from other crates 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants