[POC] Refresh on (extracted) parameter update via UI. #783
Conversation
This is the second in a series of commits "porting" the original
proof-of-concept Fornjot + `egui` integration to more recent versions
of both. Changes were required due to various non-backward-compatible
changes made to both projects.
Note: For additional context/explanation of this & upcoming commits
the original proof-of-concept commits contain additional
explanatory detail: <https://github.com/follower/Fornjot/commits/prototype-egui>
Includes a basic `Debug` implementation so it can be added to the pre-existing `Renderer` struct.
Note: The addition of `Window = egui_winit::winit::window::Window`
seemed to be required in order to pass `screen.window()`
into `egui_winit::State::new(...)`.
There may be an alternate approach which doesn't require this
if it's an issue for some reason.
Note: In an effort to make the process more readable these commits
have been split apart & don't necessarily have visible output
on-screen.
Note: This code does not yet respond to input.
Note: This code does not yet render the UI on-screen.
This code is a copy of the texture update code from: * <https://github.com/emilk/egui/blob/f807a290a422f401939bd38236ece3cf86c8ee70/egui-wgpu/src/winit.rs#L102-L136> See included comment for more details. Note: This helper function is not yet called as it lacks the forthcoming transparent clear color fix.
Note: The UI is still not yet rendered.
This is to allow interactive UI elements in future.
Tested via `WINIT_X11_SCALE_FACTOR` environment variable,
...in preparation for control via UI checkbox.
...and `wgpu_core::device: surface configuration failed: Native window is in use` error messages, in case it helps someone one day. :D
Doing this now to try doing a merge so the models have the correct macros to be parsed by my code.
Since upstream merge is still forthcoming and I want to try this now. :D
…cess. This now seems to work correctly, whereas the last time I tried to implement this functionality it didn't, IIRC. Not sure why but might be because I'm restricting it to affecting the "input handler"-related events only? And/or the `egui` monitored area might be restricted to a narrower part of the window? Anyway, this means that interacting with the UI now doesn't also affect the model view, e.g. mouse drag can change values without *also* rotating model. :)
We can now use the empty vector as indication that no parameters are available or an error occurred trying to extract them. (Rather than needing the extra complication of an `Option<>`.) (Requires updated version of the `ParamConfig` library that derives `Default`.)
(The current (uncommitted) WIP parameter config support initially stored the parameter config separately on the `Watcher` hackily but I decided it probably makes more sense on `Parameter`, so might as well do that now...) Using `BTreeMap` for sorted keys for display in the UI but it may make more sense to use the order they are specified in the source--or provide the option to do either.
This probably makes more sense to be in `load_once` but it's here for now...
This function needs to be called after the UI updates a parameter value. (This aspect is handled by upcoming commit(s).)
...due to parameter value change(s). Committing placeholder now to ease upcoming commits.
...due to parameter value change(s).
... commit 61db1dc thus presumably unbreaking the build that's been broken since then. :/
Enables display & modification of model parameter values via the UI. (Requires upcoming commits.)
...but still need to add a "redraw" button to use the updated values.
...so we can add a "redraw requested" state indicator. It would make more sense to store the state elsewhere but this will do for now.
...intended to be used after changing the model parameters. See included comment for why a manually activated redraw request is currently required. Note: Upcoming commits will make this functional.
(Note: Still needs upcoming commit which replaces the current placeholder empty function with the actual implementation.)
Use `RUST_LOG=fj_window=debug` to view...
Replaces the placeholder. With this commit the feature should now be functional.
Viewable via `RUST_LOG=fj_host=debug`.
...now that it is public.
To hopefully prevent the mentioned issue in future...
|
Okay, the new release is out, #763 is out, and I'm ready for this PR! First off, I made an attempt to rebase this branch on top of latest I didn't pay a lot of attention to correctness. My main goal was to get something quickly that I can test. Therefore, I don't know if the following problems I noticed where there in the first place, or if my rebase introduced them. First off, it doesn't look the same for me, as it does in the screenshots: The UI on the left is no longer displayed correctly. And this is what happens when I try out the spacer model: $ cargo run -- -m spacer
Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Running `target/debug/fj-app -m spacer`
found `fj::model` function: model
found arg with value attribute: "outer"
found arg with value attribute: "inner"
found arg with value attribute: "height"
Compiling spacer v0.1.0 (/home/hanno/Projects/main/fornjot/models/spacer)
Finished dev [unoptimized + debuginfo] target(s) in 0.34s
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: ParseFloatError { kind: Empty }', models/spacer/src/lib.rs:3:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtraceOther than that, there are a few minor issues, but this is a proof-of-concept, so no reason to nitpick. I'm running out of time for today, so I probably won't be doing much more here for now. I still need to check out how you get the parameters and their default values from the model, or look at the code in any kind of depth. |
|
Okay, I've now had a chance to look at this more closely, and check out how it works. I think from a UI perspective, this is a perfect starting point! However, the approach used to load the default parameters from the model is the wrong one, in my opinion. If I understand correctly, it parses the source file of the model. This duplicates the parsing code that already exists in Now that we have All of this shouldn't be seen as a complaint about your work, @follower! The pull request is clearly marked as draft, and has "proof of concept" right in the title, so I certainly didn't expect you to address long-standing architectural issues. I just want to put this pull request into a wider context, and address how this work could go forward, in my opinion. What do you think, @follower? Where do you see this pull request going from here? |
|
I've opened an issue with thoughts on what a host API that could properly support this functionality could look like: #804 |
|
I've kept this pull request open, as I hoped that it could serve as a template for future work along those lines. However, due to the recent changes (see A New Direction), this feature is no longer in scope. Closing. Thanks again for your work, @follower! |
[TODO: Add some context]