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

Simple-Objects Plugin #297

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Simple-Objects Plugin #297

wants to merge 6 commits into from

Conversation

carlobrok
Copy link

This pull request contains the new csp-simple-objects plugin with a readme.
With this it is possible to load glTF 3D models, place them on celestial bodies, rotate and move each object as desired.

Things that still need to be improved:

  • Replace the rotation input boxes and the button next to them with three Euler angle sliders. This means that conversion to a quaternion is still necessary.
  • The fly to object functionality is not implemented yet, but should be trivial.
  • CosmoScout VR crashes when entering a non-existent anchor name in the editor input field.
  • It's likely that there are other bugs that I haven't found yet.

@JonasGilg JonasGilg added the new feature A feature request or a general improvement idea label Aug 19, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2889928608

  • 0 of 287 (0.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 2.942%

Changes Missing Coverage Covered Lines Changed/Added Lines %
plugins/csp-simple-objects/src/logger.cpp 0 3 0.0%
plugins/csp-simple-objects/src/utils.cpp 0 18 0.0%
plugins/csp-simple-objects/src/SimpleObject.cpp 0 95 0.0%
plugins/csp-simple-objects/src/Plugin.cpp 0 171 0.0%
Totals Coverage Status
Change from base Build 2696156115: -0.05%
Covered Lines: 491
Relevant Lines: 16691

💛 - Coveralls

Copy link
Member

@Schneegans Schneegans left a comment

Choose a reason for hiding this comment

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

Thank you very much! This looks pretty good already. I'll do some thorough tests and report any issues I find. For now, I added some remarks to the code.

You should also run the tools/clang-format script to apply some common formatting rules.

Edit: Make sure to unfold the hidden conversations below. GitHub does not like to show too many comments 😉.

Thank you!

@@ -0,0 +1 @@
*.drawio
Copy link
Member

Choose a reason for hiding this comment

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

This file could be dropped.

@@ -0,0 +1,21 @@
MIT License
Copy link
Member

Choose a reason for hiding this comment

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

This file could be dropped as well :)

Comment on lines +4 to +12


![](demo_image.png)






Copy link
Member

Choose a reason for hiding this comment

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

Here plenty of empty lines.

Comment on lines +11 to +14
/*background-color: rgba(230, 230, 255, 0.1);
border-bottom: 2px solid var(--cs-color-primary);
box-shadow: none;
color: var(--cs-color-text);*/
Copy link
Member

Choose a reason for hiding this comment

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

Can this be removed?

Suggested change
/*background-color: rgba(230, 230, 255, 0.1);
border-bottom: 2px solid var(--cs-color-primary);
box-shadow: none;
color: var(--cs-color-text);*/

Comment on lines +110 to +119
* Funktionalität für rückwärts tabben durch prevDiv und this.tabRevMap,
* aber rückwärts funktioniert nicht.. :(
*
* Weil: "shift+tab" triggert keypress event nicht.
* -> nur "tab" (keyCodwe == 9) triggert keypress event.
*
* keydown wird durch "shift" getriggert, aber nicht durch "tab"?!
* -> während shift gedrückt ist kommt auch kein keypress event von tab. WARUM?!
*
* Ist das irgendwo im code deaktiviert oder ist das hier eine ALTE VERSION?
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 write this in english? 😉

glm::dquat qRot;
glm::dvec3 lastSurfaceNormal;

bool editEnabled = false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool editEnabled = false;
bool mEditEnabled = false;

Comment on lines +81 to +118


/*
/// A single satellite within the Solar System.
class Satellite : public cs::scene::CelestialBody {
public:
Satellite(Plugin::Settings::SimpleObject const& config, std::string const& anchorName,
VistaSceneGraph* sceneGraph, std::shared_ptr<cs::core::Settings> settings,
std::shared_ptr<cs::core::SolarSystem> solarSystem);

Satellite(Satellite const& other) = delete;
Satellite(Satellite&& other) = default;

Satellite& operator=(Satellite const& other) = delete;
Satellite& operator=(Satellite&& other) = delete;

~Satellite() override;

void update(double tTime, cs::scene::CelestialObserver const& oObs) override;

void setSun(std::shared_ptr<const cs::scene::CelestialObject> const& sun);

// interface of scene::CelestialBody ---------------------------------------

bool getIntersection(
glm::dvec3 const& rayPos, glm::dvec3 const& rayDir, glm::dvec3& pos) const override;
double getHeight(glm::dvec2 lngLat) const override;

private:
VistaSceneGraph* mSceneGraph;
std::shared_ptr<cs::core::Settings> mSettings;
std::shared_ptr<cs::core::SolarSystem> mSolarSystem;
std::unique_ptr<VistaTransformNode> mAnchor;
std::unique_ptr<cs::graphics::GltfLoader> mModel;
std::shared_ptr<const cs::scene::CelestialObject> mSun;
}; */


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*
/// A single satellite within the Solar System.
class Satellite : public cs::scene::CelestialBody {
public:
Satellite(Plugin::Settings::SimpleObject const& config, std::string const& anchorName,
VistaSceneGraph* sceneGraph, std::shared_ptr<cs::core::Settings> settings,
std::shared_ptr<cs::core::SolarSystem> solarSystem);
Satellite(Satellite const& other) = delete;
Satellite(Satellite&& other) = default;
Satellite& operator=(Satellite const& other) = delete;
Satellite& operator=(Satellite&& other) = delete;
~Satellite() override;
void update(double tTime, cs::scene::CelestialObserver const& oObs) override;
void setSun(std::shared_ptr<const cs::scene::CelestialObject> const& sun);
// interface of scene::CelestialBody ---------------------------------------
bool getIntersection(
glm::dvec3 const& rayPos, glm::dvec3 const& rayDir, glm::dvec3& pos) const override;
double getHeight(glm::dvec2 lngLat) const override;
private:
VistaSceneGraph* mSceneGraph;
std::shared_ptr<cs::core::Settings> mSettings;
std::shared_ptr<cs::core::SolarSystem> mSolarSystem;
std::unique_ptr<VistaTransformNode> mAnchor;
std::unique_ptr<cs::graphics::GltfLoader> mModel;
std::shared_ptr<const cs::scene::CelestialObject> mSun;
}; */

Comment on lines +14 to +16
namespace csp::simpleobjects {

namespace utils {
Copy link
Member

Choose a reason for hiding this comment

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

These could be merged, I guess.

Suggested change
namespace csp::simpleobjects {
namespace utils {
namespace csp::simpleobjects::utils {

Comment on lines +28 to +30
} //namespace utils

} // namespace csp::simplobjects
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} //namespace utils
} // namespace csp::simplobjects
} // namespace csp::simplobjects::utils

Comment on lines +20 to +26
glm::dvec3 getSurfaceNormal(const glm::dvec2 &lngLat, std::shared_ptr<cs::scene::CelestialBody> &body, const double offset = 0.9F);

// wrapper around cs::utils::convert::toCartesian for easier
// access to position from getSurfaceNormal()
glm::dvec3 getLngLatPositionOnBody(const glm::dvec2 &lngLat, std::shared_ptr<cs::scene::CelestialBody> &body);

glm::dquat normalToRotation(const glm::dvec3 &normal);
Copy link
Member

Choose a reason for hiding this comment

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

By convention, the const& notation is used in CosmoScout. Also, the const before the double is not required and the shared_ptr could be pass as const& as well.

Suggested change
glm::dvec3 getSurfaceNormal(const glm::dvec2 &lngLat, std::shared_ptr<cs::scene::CelestialBody> &body, const double offset = 0.9F);
// wrapper around cs::utils::convert::toCartesian for easier
// access to position from getSurfaceNormal()
glm::dvec3 getLngLatPositionOnBody(const glm::dvec2 &lngLat, std::shared_ptr<cs::scene::CelestialBody> &body);
glm::dquat normalToRotation(const glm::dvec3 &normal);
glm::dvec3 getSurfaceNormal(glm::dvec2 const& lngLat, std::shared_ptr<cs::scene::CelestialBody> const& body, double offset = 0.9F);
// wrapper around cs::utils::convert::toCartesian for easier
// access to position from getSurfaceNormal()
glm::dvec3 getLngLatPositionOnBody(glm::dvec2 const& lngLat, std::shared_ptr<cs::scene::CelestialBody> const& body);
glm::dquat normalToRotation(glm::dvec3 const& normal);

@Schneegans
Copy link
Member

I played around with the plugin! Most of the time, it works flawlessly. However, in addition to the list of issues you posted above, I discovered some further things:

  • Saving a scene does not seem to work.
    1. Place some objects.
    2. Call CosmoScout.callbacks.core.save("test.json")
    3. Restart CosmoScout VR
    4. Call CosmoScout.callbacks.core.load("test.json")
    5. The previously placed objects are not there.
  • The sun light direction is not updated
  • There seems to be a minimum distance for the object location picking to be available. For me, this is a bit confusing. Is there a reason for it not being always available?
  • Sometimes, objects become transparent. They may actually not be transparent but the draw order could be wrong. This is not-so-easy to reproduce, however, here's a possibility:
    1. Start cosmoscout with the example configuration (avocado & curiosity on Mars).
    2. Uncheck Advanced Settings -> Atmospheres -> Enabled. This will make the issue easier to spot.
    3. Fly to Earth and land on the surface, let's say in Italy.
    4. Use the edit button and then the surface picking to place curiosity somewhere in Italy.
    5. You may have to adjust the size to see it, let's insert 100.
    6. Observe the stars through curiosity 😉
    7. If you save the object, the issues persists. However, if you edit it again, it looks fine. But only until you save it again.

@Schneegans Schneegans changed the title 🎉🎨 New simple objects plugin added Simple-Objects Plugin Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature A feature request or a general improvement idea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants