mirror/OpenSpace
1
0
Fork 0
mirror of https://github.com/OpenSpace/OpenSpace.git synced 2026-05-07 03:49:43 -05:00
Code Issues Packages Projects Releases Wiki Activity

Add some comments

Browse Source
This commit is contained in:
Alexander Bock
2017-05-11 23:25:13 -04:00
parent 00ce0dd56f
commit 3a6f2a54e5
4 changed files with 41 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
modules/touch/include/TouchInteraction.h
+14 -2
View File
@@ -47,6 +47,9 @@
#include <list>
// @COMMENT It's better to use strongly-typed enums here:
// enum class Type { Rot = 0, Pinch, Pan, Roll, Pick };
// #define's leak into other parts of the program, especially if they are defined in header files
#define ROT 0
#define PINCH 1
#define PAN 2
@@ -55,6 +58,8 @@
namespace openspace {
// @COMMENT These structs are defined in the openspace namespace; it would be better to place that in either
// a subnamespace or in the Touchinteraction class
struct VelocityStates {
glm::dvec2 orbit;
double zoom;
@@ -78,22 +83,29 @@ struct FunctionData {
LMstat stats;
};
// @COMMENT Double definition
#define ROT 0
#define PINCH 1
#define ROLL 2
#define PAN 3
#define PICK 4
// @COMMENT This is also polluting the openspace namespace
using Point = std::pair<int, TUIO::TuioPoint>;
class TouchInteraction : public properties::PropertyOwner
{
public:
TouchInteraction();
// @COMMENT The destructor doesn't do anything, so it could be deleted
~TouchInteraction();
void update(const std::vector<TUIO::TuioCursor>& list, std::vector<Point>& lastProcessed);
bool gui(const std::vector<TUIO::TuioCursor>& list);
// @COMMENT How many of these functions have to be public, and could be made private instead?
void update(const std::vector<TUIO::TuioCursor>& list, std::vector<Point>& lastProcessed);
// @COMMENT all of the function names here are not very descriptive. Especially
// when it comes to the return values
bool gui(const std::vector<TUIO::TuioCursor>& list);
void manipulate(const std::vector<TUIO::TuioCursor>& list);
void trace(const std::vector<TUIO::TuioCursor>& list);
int interpret(const std::vector<TUIO::TuioCursor>& list, const std::vector<Point>& lastProcessed);
modules/touch/src/TouchInteraction.cpp
+11
View File
@@ -54,6 +54,7 @@ namespace {
}
using namespace TUIO;
// @COMMENT Instead of using namespace openspace, it's better to just open the namespace instead
using namespace openspace;
TouchInteraction::TouchInteraction()
@@ -222,6 +223,7 @@ void TouchInteraction::manipulate(const std::vector<TuioCursor>& list) {
}
// Update the camera state
// @COMMENT Do you have to make a copy of the camera object here? You are not using it afterwards
Camera cam = *(ptr->camera);
cam.setPositionVec3(camPos);
cam.setRotation(globalCamRot * localCamRot);
@@ -239,6 +241,7 @@ void TouchInteraction::manipulate(const std::vector<TuioCursor>& list) {
glm::dvec3 camPos = ptr->camera->positionVec3();
glm::dvec3 selectedPoint = (ptr->node->rotationMatrix() * ptr->selectedPoints.at(x)) + ptr->node->worldPosition();
double h = minStep * glm::distance(camPos, selectedPoint);
// @COMMENT Is it necessary to make the allication every time? It might better to only allocate the memory once (preferrably on the heap)
double* dPar = new double[ptr->nDOF];
for (int i = 0; i < ptr->nDOF; ++i) {
dPar[i] = par[i];
@@ -268,6 +271,7 @@ void TouchInteraction::manipulate(const std::vector<TuioCursor>& list) {
const int nFingers = list.size();
int nDOF = std::min(nFingers * 2, 6);
// @COMMENT Better to use a std::vector<double>(nDOF, 0.0) here and use par.data() where you need to pass the * to other functions. Then you can remove the next for loop, too
double* par = new double[nDOF];
for (int i = 0; i < nDOF; ++i) { // initial values of q or 0.0? (ie current model or no rotation/translation)
par[i] = 0.0;
@@ -331,6 +335,7 @@ void TouchInteraction::trace(const std::vector<TuioCursor>& list) {
selectableNodes.push_back(node);
//glm::dvec2 res = OsEng.windowWrapper().currentWindowResolution();
// @COMMENT ^_^
double aspectRatio = 1.88; //res.x/res.y;
glm::dquat camToWorldSpace = _camera->rotationQuaternion();
glm::dvec3 camPos = _camera->positionVec3();
@@ -385,6 +390,7 @@ void TouchInteraction::trace(const std::vector<TuioCursor>& list) {
// Interprets the input gesture to a specific interaction
int TouchInteraction::interpret(const std::vector<TuioCursor>& list, const std::vector<Point>& lastProcessed) {
// @COMMENT #include <ghoul/misc/invariants.h> Then you can use ghoul_precondition as an assertion to check if, for example, list is not empty
double dist = 0;
double lastDist = 0;
TuioCursor cursor = list.at(0);
@@ -627,6 +633,11 @@ void TouchInteraction::unitTest() {
std::vector<TuioCursor> lastFrame;
lastFrame.push_back(TuioCursor(0, 10, 0.45, 0.4)); // session id, cursor id, x, y
lastFrame.push_back(TuioCursor(1, 11, 0.55, 0.6));
// @COMMENT Alternative way of specifying:
// std::vector<TuioCursor> lastFrame = {
// { (0, 10, 0.45, 0.4 },
// { (1, 11, 0.55, 0.6 }
// };
std::vector<TuioCursor> currFrame;
currFrame.push_back(TuioCursor(0, 10, 0.2, 0.6)); // (-0.6,0)
currFrame.push_back(TuioCursor(1, 11, 0.8, 0.4)); // (0.6, 0)
modules/touch/touchmodule.cpp
+12 -1
View File
@@ -46,7 +46,10 @@ namespace {
namespace openspace {
// @COMMENT What does this do?
TuioEar TouchModule::*ear;
// @COMMENT Definining this globally makes it potentially dangerous. Is it necessary or better to put into the class?
TouchInteraction* touch;
bool TouchModule::gotNewInput() {
@@ -73,14 +76,18 @@ bool TouchModule::gotNewInput() {
if (list.size() == 0 && lastProcessed.size() == 0 && ear->tap()) {
TuioCursor c = ear->getTap();
list.push_back(c);
// @COMMENT You can use:
// lastProcessed.emplace_back(c.getSessionID(), c.getPath().back());
lastProcessed.push_back(std::make_pair(c.getSessionID(), c.getPath().back()));
touch->tap();
return true;
}
// Return true if we got new input
// @COMMENT You can use !list.empty()
if (list.size() == lastProcessed.size() && list.size() > 0) {
bool newInput = true;
// @COMMENT Why can you use for_each without std:: ? It seems like there is a using namespace std somewhere
for_each(lastProcessed.begin(), lastProcessed.end(), [this, &newInput](Point& p) {
std::vector<TuioCursor>::iterator cursor = find_if(list.begin(), list.end(), [&p](const TuioCursor& c) { return c.getSessionID() == p.first; });
double now = cursor->getPath().back().getTuioTime().getTotalMilliseconds();
@@ -98,7 +105,9 @@ bool TouchModule::gotNewInput() {
TouchModule::TouchModule()
: OpenSpaceModule("Touch")
{
// @COMMENT If TuioEar and TouchInteraction don't have any dependencies and if they
// can be initialized immediately and you don't need to register a callback
// Even better would be to not even use pointers but direct member instances instead
OsEng.registerModuleCallback(
OpenSpaceEngine::CallbackOption::Initialize,
[&]() {
@@ -134,6 +143,8 @@ TouchModule::TouchModule()
// update lastProcessed
lastProcessed.clear();
for (const TuioCursor& c : list) {
// @COMMENT You can use:
// lastProcessed.emplace_back(c.getSessionID(), c.getPath().back());
lastProcessed.push_back(std::make_pair(c.getSessionID(), c.getPath().back()));
}
touch->unitTest();
modules/touch/touchmodule.h
+4
View File
@@ -33,18 +33,22 @@
namespace openspace {
// @COMMENT This is defined in global namespace, maybe place in separate namespace or inside the TouchModule?
using Point = std::pair<int, TUIO::TuioPoint>;
class TouchModule : public OpenSpaceModule {
public:
TouchModule();
// @COMMENT better name: hasNewInput
bool gotNewInput();
// @COMMENT Does it makes sense to make this available as a public variable?
TuioEar* ear;
private:
// @COMMENT list is a very undescriptive name
std::vector<TUIO::TuioCursor> list;
std::vector<Point> lastProcessed; // contains an id and the TuioPoint that was processed last frame