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

[WIP] Package audit and few improvements #52

Open
wants to merge 54 commits into
base: master
Choose a base branch
from

Conversation

raphaelroosz
Copy link

Hello,

Since this is SAB time, it might be nice to publish my work even if still in progress.
Among those:

  1. audit of package when loaded => it allow to improve the application and packages
  2. two pass import of packages => avoid lot of markers that cannot be disabled (in red) or need for a "aaaa.xml"
  3. editable mumble link => no need to have gw2 open to develop
  4. crash report => no need launch from a terminal to have information and users can share the error file.
  5. UI vs background thread
  6. few other things

Feel free to review or accept. I am certain there are some part that needs improvements.

moi added 30 commits February 23, 2024 17:34
…id seing nothing when far from both ends + implement routes
…nts - next goal is to have a view per element with definition/configuration/state in same file for each component
… needs to be build from reference which is the Category. Any future piece of code has to take this into account. If anything dynamic occurs in Category, no loading/saving can take place elsewhere
… works with signals (messages) - need to review all features
…t packages dependancies (faster compilation intended in the end)
…t packages dependancies (faster compilation intended in the end)
@raphaelroosz
Copy link
Author

Sadly, I don't know how to validate the windows part before PR.

@coderedart
Copy link
Owner

I do like the validation of packs and other parts. But it feels like this PR moves more of the marker stuff into the main overlay, while, I want it to be more separated.

I am still working on the plugin system design, but I plan to first move markers part into a wasm plugin (and out of this repo). This will allow markers part to be updated separately (and often with tiny improvements), while the main overlay can remain barebones.

Would it be possible to introduce back the MarkerManager (which abstracts away all the marker parts from the main overlaly)?

@raphaelroosz
Copy link
Author

raphaelroosz commented Apr 24, 2024

Let's trace back what leads to current result.

Initial state:
1 - Everything is done in UI.
2 - There is a will to separate the code into subcomponents.

In UI (one of the most complicated type of application), there is usually a UI thread and one or more back-end thread. Those two kinds communicate asynchronously ("signals", or "messages") to not freeze the UI.

Obvious first step is thus to identify what freeze or could freeze the UI and extract it to another thread.
Knowing that:

  • In current code, display of categories and loading of packages are done in the same object.
  • Advertised goal is to have WASM, which afaik is bad with multi-threading.
  • There are two ways to handle multiple threads working together: either mutex lock everywhere, or true parallelism (no lock) + signals.
    I chose the later. The Marker Manager is thus split in two parts that communicate with each others with signals: one run in UI (to display menus), the other in back-end that are time consuming (save/load files).
    => The marker manager is still there, split into a PackageDataManager and PackageUIManager.

In my opinion the future components (plugin or else) would need to run on either the UI world or the back-end world. If both, then it would need to become two components (or more) communicating with each others.
Since components is what you are working on, I did not push too far in that direction and put every definition of those messages into a single place. That way it's easier to see what would be the required messages for who and what.

I also tried to simplify the dep graph (check on master vs branch)
cargo depgraph --all-deps --workspace-only | dot -Tpng > graph.png

All in all, that is why on this PR the jokolay module act as the application which pilot everything.
And true, part of those underlying modules are imported into the "application" and break abstraction. To solve this, I need to work with you to know the wished direction for abstractions. As an example, there is a need route messages by encapsulating those from a component to another being agnostic to content.
=> this is where this PR stands. Doing further work without at least communicating about what I am doing, in my opinion, would be a fork of the project without possible reintegration. Which I do not want.

Under the assumption of separation between UI and back, there would be "content provider"s and "renderer"s (or any kind of service).
A "renderer" having either direct access to draw or delegating it to a global renderer. In which case it needs to know the state of the world to render it at each frame.
Other question is: do we want a bad "plugin" to freez the UI by giving it full access to the draw feature ?
A global renderer would need to know what to draw and how (state of the world). How to abstract composition on a per plugin basis ?
Same questions for configuration: we need a "form" to submit information to the component. That information could be something else than configuration.
Same questions for ... everything.
You did start on the component territory, to me it is a bit uncharted. So we would need specification or a PoC to solve the above problems :)

I am totally fine if PR is rejected. You can see it as a PoC of a working UI vs backend architecture.

Out of topic:
since some packages are very dirty, I believe there would be a need to have the future "package plugin" that load them to itself allow "plugins" to fix the package before final import.
A long chain of fix before a sane import.
It can be done safely in isolation like https://github.com/dtolnay/watt advertises.
That way it's easier to change order or add/remove/replace a fix.

@raphaelroosz
Copy link
Author

P.S.: let me amend the "I am totally fine if PR is rejected" part: I am expecting this PR to be rejected (too many changes), unless you judge it to be in a satisfying direction.
It would need to be break down and merged bit by bit.

@raphaelroosz
Copy link
Author

Hello,
broke many things. Fixed them all (I think).

It's basically the same thing, isolated per responsibility. Not knowing the interface for plugins I went to something generic.
Still work in progress on the "requirements" to avoid Send + Sync.

Each responsibility is in a component with an interface on how to communicate to others (which, how and in what order).
When a component has requirements it should be in the same thread (world). Data is passed synchronously (for technical reason, the channels are async and would need to be changed: remove dummy Sync + Send traits) . Otherwise it's notifications between threads asynchronously.

This would allow for future plugins/components to be completely interoperable. As long as relationships are kept a substitute can be introduced easily. Traits and interface can of course change but shall require to be enforced everywhere. Relationships could be stated into a manifest for plugins, having one third party plugin depending on third party plugins.

I did not change much on renderer, but there is a clear dependency with first class primitives such as marker and trails in it. Package reading + managing + rendering are intertwined.

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