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.

Sign up for GitHub

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

Jump to bottom

Fix VRPN plugin, add floor projection and OpenDeck plugin #616

Merged
merged 1 commit into from
Nov 28, 2018
Merged

Fix VRPN plugin, add floor projection and OpenDeck plugin #616

merged 1 commit into from
Nov 28, 2018

Conversation

chevtche
Copy link
Contributor

No description provided.

tribal-tec
tribal-tec reviewed Nov 26, 2018
View reviewed changes
plugins/CMakeLists.txt
@@ -12,12 +12,16 @@ if(BRAYNS_NETWORKING_ENABLED)
add_subdirectory(Rockets)
endif()

option(BRAYNS_VRPN_ENABLED "Activate VRPN plugin" OFF)
if(BRAYNS_OPENDECK_ENABLED)
Copy link
Contributor

Choose a reason for hiding this comment

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

The option should go here then, not in the top-level CMake?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

ospray_modules/opendeck/ispc/cylindricStereoTracked/CylindricStereoTrackedCamera.ispc
pixelPos.x = 2.0f * OPENDECK_RADIUS * (screen.x - 0.5f);
pixelPos.y = 0.0f;
pixelPos.z = -OPENDECK_RADIUS + OPENDECK_RADIUS * screen.y;
// pixelPos.z = -OPENDECK_RADIUS * screen.y;
Copy link
Contributor

Choose a reason for hiding this comment

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

that's the top-left vs bottom-left thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

plugins/OpenDeck/OpenDeckPlugin.cpp Outdated

void OpenDeckPlugin::init(PluginAPI* api)
{
_api = api;
Copy link
Contributor

Choose a reason for hiding this comment

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

that member is actually not needed. It should be once your plugin gets unloaded to remove the buffers again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

plugins/OpenDeck/OpenDeckPlugin.h Outdated
public:
OpenDeckPlugin(const Vector2ui& wallRes, const Vector2ui& floorRes);

virtual void init(PluginAPI* api);
Copy link
Contributor

Choose a reason for hiding this comment

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

final, not virtual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

plugins/VRPN/VRPNPlugin.h Outdated
~VRPNPlugin();

virtual void init(PluginAPI* api);
Copy link
Contributor

Choose a reason for hiding this comment

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

final, not virtual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

plugins/VRPN/VRPNPlugin.h Outdated
@@ -46,7 +48,7 @@ class VRPNPlugin : public ExtensionPlugin

private:
PluginAPI* _api = nullptr;
Camera& _camera;
Camera* _camera;
Copy link
Contributor

Choose a reason for hiding this comment

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

use _api->getCamera() instead, rather than having this pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

karjonas
karjonas reviewed Nov 26, 2018
View reviewed changes
plugins/OpenDeck/OpenDeckPlugin.cpp Outdated
"native OpenDeck resolution.");
}

const float scaling = (argc == 2) ? atof(argv[1]) : 1.0f;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use std::stof instead since atof causes undefined behaviour on garbage input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

plugins/OpenDeck/OpenDeckPlugin.cpp
{
namespace
{
constexpr uint32_t openDeckWallResX = 11940u;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for u suffix.

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 like the suffix !!

ospray_modules/opendeck/ispc/cylindricStereoTracked/CylindricStereoTrackedCamera.cpp
@@ -25,6 +25,11 @@ namespace ospray
{
namespace
{
constexpr uint8_t leftWall = 0u;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for u suffix.

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 like the suffix !!

tribal-tec
tribal-tec reviewed Nov 26, 2018
View reviewed changes
plugins/OpenDeck/CMakeLists.txt Outdated

set(BRAYNSOPENDECK_HEADERS OpenDeckPlugin.h)
set(BRAYNSOPENDECK_SOURCES OpenDeckPlugin.cpp)
set(BRAYNSOPENDECK_LINK_LIBRARIES PRIVATE vmmlib braynsPluginAPI)
Copy link
Contributor

Choose a reason for hiding this comment

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

use braynsCommon instead of vmmlib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tribal-tec
tribal-tec reviewed Nov 27, 2018
View reviewed changes
CMakeLists.txt Outdated
@@ -85,6 +85,7 @@ endif()
common_find_package(Deflect)
if(TARGET Deflect)
option(BRAYNS_DEFLECT_ENABLED "Activate streaming to display wall" ON)
option(BRAYNS_OPENDECK_ENABLED "Activate opendeck plugin and module" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

revert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tribal-tec
tribal-tec reviewed Nov 27, 2018
View reviewed changes
ospray_modules/CMakeLists.txt
@@ -24,7 +24,6 @@ if(BRAYNS_OPTIX_ENABLED)
endif()

# OpenDeck module
option(BRAYNS_OPENDECK_ENABLED "Activate OpenDeck module" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

revert. should work when the option stays here also for the plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tribal-tec
tribal-tec reviewed Nov 27, 2018
View reviewed changes
Copy link
Contributor

@tribal-tec tribal-tec left a comment

Choose a reason for hiding this comment

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

A more elaborate commit message? "Fix VRPN plugin, add floor projection and OpenDeckPlugin

@chevtche chevtche force-pushed the master branch 3 times, most recently from 221dad3 to 17d7225 Compare November 27, 2018 11:15
@chevtche
Copy link
Contributor Author

retest this please

@chevtche
Copy link
Contributor Author

Approve !!

@tribal-tec
Copy link
Contributor

Rebase!!

@tribal-tec tribal-tec changed the title Enabling floor projection. Fix VRPN plugin, add floor projection and OpenDeck plugin Nov 27, 2018
tribal-tec
tribal-tec previously approved these changes Nov 27, 2018
View reviewed changes
@chevtche
Copy link
Contributor Author

retest this please

1 similar comment
@chevtche
Copy link
Contributor Author

retest this please

@chevtche
Copy link
Contributor Author

APROOOOOOVE !!!!!!

@chevtche chevtche merged commit cbbca19 into BlueBrain:master Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karjonas karjonas karjonas left review comments

@tribal-tec tribal-tec tribal-tec approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chevtche @tribal-tec @karjonas