-
Notifications
You must be signed in to change notification settings - Fork 12
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
Cameraz #156
base: master
Are you sure you want to change the base?
Cameraz #156
Conversation
…ers (temporary to common/include/camera directory)
…ific range type & projection matrix type constraints, update ILinearProjection and CCubeProjection - now I can pair IProjection with a gimbal
…urces, save main.cpp tests, add TODO comments
…s with abstract manipulate method - expect virtual event & a gimbal. Update CGimbal with new manipulation & set methods, start replacing CCamera content with new gimbal & controller
…from CCamera, mark TODOs for tomorrow
… tomorrow - make CCamera use gimbal & treat as FPS controller
…tion with isLeftHanded method, update sources & some of matrices to HLSL - the pure virtual controller manipulate method now takes a gimbal + span of virtual events
…h a setter & get with a getter therefore determine at set time handness (compute determinant only once after the matrix gets changed!), update main.cpp & sources - time to test it
…tance virtual, init time stamps with defaults
…tation (TODO: constraints for type of camera + I broke continuity of events signaling & need those keysPressed bools I guess)
common/include/ICamera.hpp
Outdated
ICamera(core::smart_refctd_ptr<typename Traits::gimbal_t>&& gimbal, core::smart_refctd_ptr<typename Traits::projection_t> projection, const float32_t3& target = {0,0,0}) | ||
: base_t(core::smart_refctd_ptr(gimbal)), m_projection(core::smart_refctd_ptr(projection)) { setTarget(target); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that we want gimbals to be freely allocatable on the heap and refcounted, I'm thinking that camera should OWN them and they should live inside the camera as a member (and not inherit from IReferenceCounted)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
common/include/ICamera.hpp
Outdated
inline void setPosition(const float32_t3& position) | ||
{ | ||
const auto* gimbal = base_t::m_gimbal.get(); | ||
gimbal->setPosition(position); | ||
recomputeViewMatrix(); | ||
} | ||
|
||
inline void setTarget(const float32_t3& position) | ||
{ | ||
m_target = position; | ||
|
||
const auto* gimbal = base_t::m_gimbal.get(); | ||
auto localTarget = m_target - gimbal->getPosition(); | ||
|
||
// TODO: use gimbal to perform a rotation! | ||
|
||
recomputeViewMatrix(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may I suggest a trick to avoid recomputing view matrix all the time?
keep a "dirty flag" / incrementing counter in the gimbals that increments after every gimbal manipulation.
then make the camera track the last counter values of all gimbals during getViewMatrix
, then you know if the view matrix is stale or not and whether to call recomputeViewMatrix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the position and target setting should happen purely based on gimbal manipulation, its a bit of code duplication and inversion of control for anyone to do it through the camera
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep a "dirty flag" / incrementing counter in the gimbals that increments after every gimbal manipulation
done
then make the camera track the last counter values of all gimbals during getViewMatrix, then you know if the view matrix is stale or not and whether to call recomputeViewMatrix
however I changed how we model the view & updated gimbal API in latest commit, now gimbal knows when to update its view if present
common/include/CCamera.hpp
Outdated
virtual void manipulate(std::span<const typename traits_t::controller_virtual_event_t> virtualEvents) override | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess CCamera needs to be called a CFPSCamera then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
}; | ||
|
||
//! Class providing linear cube projections with projection matrix per face of a cube, each projection matrix represents a single view-port | ||
template<ProjectionMatrix T = float64_t4x4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could actually always make the projection and view matrices 64bit, there's no downsides to that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to allow for 32 bit however half precision I removed
EDIT: made them 64 bit and just used a few cast utils with my example
common/include/ICamera.hpp
Outdated
m_viewMatrix[2u] = float32_t4(zaxis, -hlsl::dot(zaxis, position)); | ||
} | ||
|
||
const core::smart_refctd_ptr<typename Traits::projection_t> m_projection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have a think whether its better to have the projection own the camera and not the camera own the projection, why?
projection needs to produce matrices/functions based off the view matrix, and you know there will only ever be one.
If you do the reverse then the projection can't know about the camera, which is rather difficult for e.g. a cubemap to produce 6 concatenated MVP matrices for each face
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its better to have the projection own the camera
you are right, on it
// Represents the camera's orthonormal basis | ||
// https://en.wikipedia.org/wiki/Orthonormal_basis | ||
float32_t3x3 m_orthonormal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only keep the quaternion, and spit out the matrix on demand, its not that expensive to compute it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also keep everything as float64_t
or at least template it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
templated, allowing for precision parameter
inline void reset() | ||
{ | ||
// TODO | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure you want this (in the gimbal that is, the camera can have it, e.g. reference/strating gimbal positions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
// Orientation of gimbal | ||
inline const glm::quat& getOrientation() const { return m_orientation; } | ||
|
||
// Orthonormal [getXAxis(), getYAxis(), getZAxis()] matrix | ||
inline const float32_t3x3& getOrthonornalMatrix() const { return m_orthonormal; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid semantical confusion (as to what your postion means) I'd also add a getMatrix()
method which would return a float64_t3x4
(a full TRS).
P.s. if you like you can even make it an operator float64_t3x4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
…otation speed factors, fix issues with camera speed while being manipulated from imguizmo controller (write thoughts about sensitivity of event magnitudes), allow for axes flip for better visibility, bind camera movement into space instead of left shift, assume imguizmo controller generates events always from World space because of what its delta TRS matrix represents (no base map), add more docs
…geometry creator bound (instead of mouse scroll), fix issues with capturing events when window has no focus (no queues & discard old ones)
…derFlags_Logarithmic for some sliders
… 61_UI/cameras.json. Make a few changes to interface classes, load demo data from the json file. TODO for tomorrow: use loaded planar projections vector to interact with the application.
…lation bugs after demo & API updates
…ar_projections_range_t>` in the demo with bound & last projection IXes, fix picking projection presets
…on by 0, fix object-to-gizmo visual alignment in a window & camera gizmo manipulation controller bugs after API updates
…ection presets, add third camera to the scene - a few minor fixes need to be applied and we start adding new type of camera (Maya incoming)
…ion to manipulate active camera in move mode
…-Examples-and-Tests into yas_cameraz
…abla-Examples-and-Tests into cameraz
… active window FBO rendering bugs in full screen mode
No description provided.