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

Issue 159 #165

Closed
wants to merge 3 commits into from
Closed

Issue 159 #165

wants to merge 3 commits into from

Conversation

ncomendant
Copy link

@ncomendant ncomendant commented Jan 7, 2024

Current progress on issue #159. Currently only has bevy::input::gamepad::GamepadEvent registered, and even that cannot change most values from its defaults.

@rewin123
Copy link
Owner

rewin123 commented Jan 7, 2024

Thanks for the PR. But I think writing wrappers is the hard way. I would suggest adding a method to achieve sending events that doesn't require a Default implementation. This can be done with manual registration of implementation default behaviour without wrapping

fn register_just_event<T : Event>(name : Stroig, create_default : Box<dyn FnMut() -> T>) {
//register creator function 
}

@ncomendant WDYT?

@ncomendant
Copy link
Author

ncomendant commented Jan 7, 2024

Yes, that would definitely be preferred. The PR is a very brittle solution and not ideal.

I think Default is not the issue, but currently events must implement Resource to be rendered by the UI (using bevy_inspector_egui::bevy_inspector::by_type_id::ui_for_resource). If it is possible to remove that limitation, then maybe we could insert the bevy events as is.

@ncomendant ncomendant marked this pull request as draft January 7, 2024 18:30
@rewin123
Copy link
Owner

rewin123 commented Jan 7, 2024

It's possible

@rewin123
Copy link
Owner

rewin123 commented Jan 7, 2024

https://github.com/rewin123/space_editor/blob/main/crates/editor_ui/src/inspector/mod.rs

Line 228

I'm from phone and can't write full example, but components reflection in inspector use undeps solution. And it can be used for any reflected structure

@naomijub
Copy link
Collaborator

naomijub commented Jan 7, 2024

I just wanted the progress to not be lost and get documented. This is why I created this extra branch. However, the solution proposed can be applied to the other PR and probably solve the Default and Resource dependency. Will try it today

@rewin123
Copy link
Owner

rewin123 commented Jan 7, 2024

And we can store events in single vector with

struct UntypedEvent {
name : String, 
value: Box<dyn Reflect>, 
create_default: Box<dyn FnMut... >, 
show_ui: Box<dyn FnMut(&mut dyn Reflect) -> UiEvents> 
... 
}
#[derive(Resouce) ]
struct AllEventStorage {
events : Vec<UntypedEvent>
}

@rewin123
Copy link
Owner

rewin123 commented Jan 7, 2024

I'm not against merging this PR, but ideally we should find a way to reduce the number of derives needed to only two - Event and Reflect (or just Event for field less struct events like "struct NoFieldEvent;")

@ncomendant
Copy link
Author

Honestly I think the Default dependency I added was never needed, since the event was stored as a Resource and being cloned anyway.

Looks like we're all on the same page. I'll open the PR again, but whatever you decide to do with it is fine by me.

@ncomendant ncomendant marked this pull request as ready for review January 7, 2024 18:49
@naomijub
Copy link
Collaborator

naomijub commented Jan 7, 2024

I'm not against merging this PR as a temporary solution, but ideally we should find a way to reduce the number of derives needed to only two - Event and Reflect (or just Event for field less struct events like "struct NoFieldEvent;")

Like I said, I didn't want progress to be lost, so I asked @ncomendant to open the PR on this branch. I loved your idea and will try it today in the other branch that I have open

@naomijub naomijub closed this Jan 7, 2024
@naomijub
Copy link
Collaborator

naomijub commented Jan 7, 2024

This PR was a documentation on the first solution to custom events UI. There is a new solution to be implemented which addresses the multiple derive dependencies

@rewin123
Copy link
Owner

rewin123 commented Jan 7, 2024

@ncomendant I apologise that my comments caused the PR to be closed T_T But all manually wrapping way is too hard for support

@rewin123
Copy link
Owner

rewin123 commented Jan 7, 2024

We can also store events in separate resources while getting rid of the Resource dependency. For example

#[derive(Resource) ]
struct ResourceWrapper<T: Event>(pub T) ;

@rewin123
Copy link
Owner

rewin123 commented Jan 7, 2024

I just realised that the PR was not about merging into main and was about saving the documentation and work done to pass on. I apologise, it turns out my comments were not for this PR, but for the events branch. Anyway, thanks for the very cool work already done

Repository owner locked and limited conversation to collaborators Jan 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants