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

226 implement data exchange with comet #227

Open
wants to merge 11 commits into
base: development
Choose a base branch
from

Conversation

DimitriDiantos
Copy link
Contributor

@DimitriDiantos DimitriDiantos commented May 14, 2024

This branch introduces the ability to connect to a Comet server, fetch engineering model data, and display it in a hierarchical tree within the application.

Key Features:
Comet Server Configuration Wizard: Easily connect to a Comet server by entering the server URL, login, and password.
Data Fetching and Visualization: Fetched data is displayed in a tree structure, showing key elements and attributes like mass.
Tree Item Selection: Supports multi-selection of items with checkboxes for efficient data handling.
How to Use:
Navigate to File > Import..., select Comet Import Wizards, and configure your server details.
Click Fetch Data to retrieve and display the data.
Screenshot 2024-08-16 143456
Screenshot 2024-08-16 143316
Screenshot 2024-08-16 143248

@DimitriDiantos DimitriDiantos linked an issue May 14, 2024 that may be closed by this pull request
Copy link
Member

@franzTobiasDLR franzTobiasDLR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, pretty cool already 🚀 Now we can see and check the model elements to be loaded from COMET!!!

For the selection where to import to, you can probably use our tree wizrad page: https://github.com/virtualsatellite/VirtualSatellite4-Core/blob/development/de.dlr.sc.virsat.uiengine.ui/src/de/dlr/sc/virsat/uiengine/ui/wizard/ATreeViewerPage.java

And the actual import to the configruation tree can then probably done with some EMF comand that recursively creates the element configurations? You can take inspiration how we implemented it with the CAD import command in core:
https://github.com/virtualsatellite/VirtualSatellite4-Core/blob/21c05a97ab73c79bf96aad96807fca7ebbbe0239/de.dlr.sc.virsat.model.extension.mechanical.ui/src/de/dlr/sc/virsat/model/extension/mechanical/ui/wizards/CadImportWizard.java#L99

Sounds good?! :)

id="de.dlr.sc.virsat.model.extension.cefx.ui.importWizards.CometImportWizard"
name="Comet Import Wizards">
<description>
Import a file from the local file system into the workspace.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is outdated isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it and wrote: CometImportWizard.

try {
openSession();
Display.getDefault().asyncExec(() -> populateTreeWithIteration(tree));
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better to not catch the generic Exception but rather specific ones that can happen at this location?

You can check the Anti-Pattern Chapter at this page for more information:
https://www.baeldung.com/java-exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for providing the link. I checked it and already fixed it :).

*/
private void populateTreeWithIteration(Tree tree) {
var siteDirectory = session.getAssembler().retrieveSiteDirectory();
var engineeringModelIid = siteDirectory.getModel().get(0).getEngineeringModelIid();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there are more than one model in COMET ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point. I've updated the code to handle multiple models by checking for their presence and implementing a selection mechanism to choose the appropriate model before proceeding.

private void populateTreeWithIteration(Tree tree) {
var siteDirectory = session.getAssembler().retrieveSiteDirectory();
var engineeringModelIid = siteDirectory.getModel().get(0).getEngineeringModelIid();
var iterationIid = siteDirectory.getModel().get(0).getIterationSetup().get(0).getIterationIid();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we do actually have multiple iterations in our COMET example...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code to handle multiple iterations by checking if more than one iteration exists and implemented a mechanism to select the appropriate iteration before proceeding.

var siteDirectory = session.getAssembler().retrieveSiteDirectory();
var engineeringModelIid = siteDirectory.getModel().get(0).getEngineeringModelIid();
var iterationIid = siteDirectory.getModel().get(0).getIterationSetup().get(0).getIterationIid();
var domainOfExpertiseIid = siteDirectory.getModel().get(0).getActiveDomain().get(0).getIid();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use the first DomainOfExperties here? The domains are System, Power, AOCS and so on, aren't they? What happens if we want to access data of another domain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the existing code, we use the first DomainOfExpertise as a default selection to retrieve data like the example of comet. This is a simplification assuming that the first domain is sufficient for the intended operation. If we need to access data from a different domain, the code would need to be extended to allow selection or iteration over the available domains rather than defaulting to the first one.


if (openIteration.isPresent()) {
openIteration.get().getElement().stream()
.filter(element -> "Space Segment".equals(element.getName()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we focused on the "Space Segement" here? What happens if there is now Space Segement in the COMET model? Maybe filter for the element that has TopLevelElement=true? (In our example that should be the CEFO Mission)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented a custom method to identify the top-level element by checking if it is not referenced by any other element, ensuring that the correct element is selected even if 'Space Segment' is not present.

/**
* Recursively adds child elements of a parent ElementDefinition to a TreeItem.
*/
private void addChildren(TreeItem parentItem, ElementDefinition parentElement) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we create a option in the wizard for what kind of data we want to import? For CEF interessting values are probably mass, power and temperature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add later options in the wizard to allow users to select the specific parameters they want to import, such as mass, power, and temperature, and modified the data fetching process to include only the selected parameters. Now, the mass will be automatically imported during the import.

import cdp4dal.SessionImpl;
import cdp4dal.dal.Credentials;
import cdp4servicesdal.CdpServicesDal;
import org.eclipse.swt.SWT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it makes sense to separate data fetching and UI? So that we do not have UI classes (SWT) in this one? Then we could move this class to the non-ui part of the plugin and test it? :)

Probably we could do that by replacing the SWT tree class with some UI independent class? Such as either a EMF object / Maybe directly a VirSat model class? Or maybe a plain java object that just accepts children objects? What do you think?

The error handling would then work with Exceptions: If there is an error (Login wrong, no Iteration or whatever) you throw a corresponding exception and catch this exception in the Wizard... Then e.g. showing a warning...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've separated the data fetching logic from the UI by creating a non-UI CometDataFetcher class that uses a TreeNode model to represent the data. The UI components now handle displaying this data and catching exceptions to show appropriate warnings, making the code more modular and testable.


Button fetchButton = new Button(container, SWT.PUSH);
fetchButton.setText("Fetch Data");
fetchButton.addSelectionListener(new SelectionAdapter() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you maybe handle the add event in a dedicated function? There you could then use an eclipse job with progress monitor... So that users get feedback that something is being done in the background?

Maybe take some inspiration from here: https://github.com/virtualsatellite/VirtualSatellite4-Core/blob/21c05a97ab73c79bf96aad96807fca7ebbbe0239/de.dlr.sc.virsat.model.extension.mechanical.ui/src/de/dlr/sc/virsat/model/extension/mechanical/ui/wizards/CadImportWizard.java#L87

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code to ensure that UI elements are accessed only on the UI thread, using syncExec to safely retrieve input values before starting the job, which prevents the Invalid thread access error and implement the Visible Progress Dialog.

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.

Implement data-exchange with Comet
2 participants