Skip to content

Comments

Split GUI into modules and separate structs#13

Merged
DerFetzer merged 6 commits intoDerFetzer:mainfrom
faern:split-gui-into-modules
Apr 7, 2025
Merged

Split GUI into modules and separate structs#13
DerFetzer merged 6 commits intoDerFetzer:mainfrom
faern:split-gui-into-modules

Conversation

@faern
Copy link
Contributor

@faern faern commented Apr 6, 2025

I'm working on a solution to #6. But gui.rs is already large. And adding all the code needed to draw a chromaticity diagram makes it explode even more. I'm currently doing it with OpenGL. So it's shaders and stuff.

I have commented earlier about the GUI struct/module being quite entangled in the core functionality of the program overall.

To combat both problems above I explore this option, to move gui.rs -> gui/mod.rs and allow sub-modules implementing the respective parts of the UI. Apart from avoiding very long files, the dependencies for each part of the UI becomes clearer. You can see on the struct members of the individual UI structs what state they hold, and you can see from the arguments to the update method what they depend on in order to perform their actions.

What do you think about going this direction? In this PR I only (so far) extract one of the program windows into its own module. I did not want to spend more time before discussing this solution with you.

Allows adding more UI modules and separate them
@faern faern force-pushed the split-gui-into-modules branch from eaaf591 to cc93834 Compare April 6, 2025 14:49
@faern faern force-pushed the split-gui-into-modules branch from cc93834 to f232f6c Compare April 6, 2025 14:49
@DerFetzer
Copy link
Owner

Thank you very much! The more modular the code is the better.

I hope the OpenGL code will be so that I can understand it, for my knowledge there is very limited. But sure: Doing it with egui primitives would probably be quite a hassle.

Do not get me wrong: The only concern is that I want to be able to continue to maintain this project in the future. This will be easier if I would understand the code. 😉

@faern
Copy link
Contributor Author

faern commented Apr 6, 2025

I went ahead and extracted the camera control UI as well. To see if this approach seemed to work. I think it turned out pretty nicely.

@faern
Copy link
Contributor Author

faern commented Apr 6, 2025

I hope the OpenGL code will be so that I can understand it, for my knowledge there is very limited. But sure: Doing it with egui primitives would probably be quite a hassle.

I'll make sure to document it! OpenGL is not always very intuitive 😂 I'm by no means an expert neither! And yes, I think I can draw it a lot nicer looking and way more performant with OpenGL. I already have the GL code for a chromaticity diagram in another project, so I hope to be able to port it somewhat simple 🤞

@faern
Copy link
Contributor Author

faern commented Apr 6, 2025

@DerFetzer I'm glad you like this direction! How would you prefer to move ahead? Do you want to merge this as is, to not make the PR too big. Or do you want me to extract as much as possible in this one PR?

@DerFetzer
Copy link
Owner

Since you seem to be quite motivated at working on this I don't mind a big PR. Problem would be if it would linger for a long time but I suppose the rest will be straight forward as well.

@faern
Copy link
Contributor Author

faern commented Apr 6, 2025

Ok! Yeah, I also don't want this to linger. Because it touches so much code that it will cause any other change to bitrot instantly. It would be nice for everyone if this can be merged asap. But I'll work a bit more today on migrating more modules then, and ping you when I feel ready. It does not have to be everything in one go.

@faern
Copy link
Contributor Author

faern commented Apr 6, 2025

Done! Now I have moved all the windows that lived under the draw_windows method at least. I think this is good enough for now and can be reviewed (and merged?). As we said above, we want this to not become stale. Better to make smaller incremental changes.

In the last commit the payoff of this design became more apparent! Three fields were moved from the big Gui struct into the CameraWindow struct. Allowing isolating more state to the place that uses it ✨

@DerFetzer DerFetzer merged commit 130e01a into DerFetzer:main Apr 7, 2025
3 checks passed
@faern faern deleted the split-gui-into-modules branch June 7, 2025 19:04
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.

2 participants