Cameraz by AnastaZIuk · Pull Request #995 · Devsh-Graphics-Programming/Nabla

AnastaZIuk · 2026-02-02T17:45:32Z

Introduces the new camera stack as Nabla::ext::Cameras.

…s - we were never be able to do ourMatrixT = glmMatrixT and its important since a lot of utils create glmMatrixT actually

…atrix xD remove the = operator glm -> to ours

…ls.hlsl from #760

…H (ambiguity dependent type issues), reference #760

…ng/Nabla/pull/760/files#r1816728485 for #760 PR, update examples_tests submodule

…odule

… were missing an equivalent of E_KEY_CODE. Update examples_tests submodule

…anguage/inline and afterwards https://gudok.xyz/inline/ please

Erfan-Ahmadi · 2026-09-04T11:00:53Z

+template<typename T, uint32_t N>
+using camera_vector_t = vector<T, N>;
+
+template<typename T, uint32_t N, uint32_t M>
+using camera_matrix_t = matrix<T, N, M>;
+
+template<typename T>
+using camera_quaternion_t = math::quaternion<T>;


ok these are just aliases to hlsl counterpart.
I don't see any point in having them. Is there a distinction between a camera matirx and a regular matrix? is there a distinction between camera quaternion and regular quaternion? this is confusing. I'd drop them. and if you want type safety, you are not going to get that with a simple typedef/using.

Erfan-Ahmadi · 2026-09-04T11:01:24Z

+using camera_quaternion_t = math::quaternion<T>;
+
+template<typename T>
+struct SRigidTransformComponents


Again, decide whether you want to template on float_t or not. a lot of places are assuming float64_t

Erfan-Ahmadi · 2026-09-04T11:01:51Z

+
+struct SCameraViewRigDefaults final
+{
+    static constexpr double DegreesToRadians = numbers::pi<double> / 180.0;


small nitpicking: float64 instead of double

Erfan-Ahmadi · 2026-09-04T11:04:48Z

+    using gimbal_event_t = core::CVirtualGimbalEvent;
+    using encode_keyboard_code_t = ui::E_KEY_CODE;
+    using encode_mouse_code_t = ui::E_MOUSE_CODE;
+    using encode_imguizmo_code_t = gimbal_event_t::VirtualEventType;


has nothing to do with imguizmo directly, I'd rename it to avoid confusion.
these are just scripted gimbal events types that don't necessarily come from inputs

Erfan-Ahmadi · 2026-09-04T11:06:08Z

+    }
+};
+
+class CGimbalBindingLayoutStorage : public IGimbalBindingLayout


what are you gaining with this inheritance? parent has decided unordered map and all function signatures and there is only one reasonable impl for the storage. I'd merge these two and not overcompilicate things

Erfan-Ahmadi · 2026-09-04T11:08:02Z

+    struct CKeyInfo
+    {
+        union
+        {
+            encode_keyboard_code_t keyboardCode;
+            encode_mouse_code_t mouseCode;
+            encode_imguizmo_code_t imguizmoCode;
+        };
+
+        CKeyInfo(encode_keyboard_code_t code) : keyboardCode(code), type(Keyboard) {}
+        CKeyInfo(encode_mouse_code_t code) : mouseCode(code), type(Mouse) {}
+        CKeyInfo(encode_imguizmo_code_t code) : imguizmoCode(code), type(Imguizmo) {}
+
+        BindingDomain type;
+    };


unused struct

Erfan-Ahmadi · 2026-09-04T11:15:26Z

+        BindingDomain type;
+    };
+
+    struct CHashInfo


CHashInfo? what are you hashing, I don't get the name.
I get that you want a map from input -> event.

also it's a bit unclear, if we have a RIGHT event, that could map to a LEFT event? I think the virtual events should map 1:1? or maybe we should treat them differently.

Also it looks like dynamic data (active) is being stored next to the bindnig which causes you to "sanitize" the map. also sanitizeMapping is duplicated verbatim somewhere else.

imo the mapping from input event -> camera event should be pretty static and the state like "active" should be managed separately.

Erfan-Ahmadi · 2026-09-04T11:18:05Z

+void CGimbalInputBinder::copyActiveBindingsFromLayout(const IGimbalBindingLayout& layout)
+{
+    updateKeyboardMapping([&](auto& map) { map = sanitizeMapping(layout.getKeyboardVirtualEventMap()); });
+    updateMouseMapping([&](auto& map) { map = sanitizeMapping(layout.getMouseVirtualEventMap()); });
+    updateImguizmoMapping([&](auto& map) { map = sanitizeMapping(layout.getImguizmoVirtualEventMap()); });
+}
+
+void CGimbalInputBinder::copyBindingLayoutFrom(const IGimbalBindingLayout& layout)
+{
+    copyActiveBindingsFromLayout(layout);
+}
+
+void CGimbalInputBinder::copyActiveBindingsToLayout(IGimbalBindingLayout& layout) const
+{
+    layout.updateKeyboardMapping([&](auto& map) { map = sanitizeMapping(getKeyboardVirtualEventMap()); });
+    layout.updateMouseMapping([&](auto& map) { map = sanitizeMapping(getMouseVirtualEventMap()); });
+    layout.updateImguizmoMapping([&](auto& map) { map = sanitizeMapping(getImguizmoVirtualEventMap()); });
+}
+
+void CGimbalInputBinder::copyBindingLayoutTo(IGimbalBindingLayout& layout) const
+{
+    copyActiveBindingsToLayout(layout);
+}


wait what?! these are just duplicated functions with the same signature.
also sanitizeMapping is weirdly duplicated here, and would not be needed if we seperated state from bindingconfig (I think)

Erfan-Ahmadi · 2026-09-04T11:27:14Z

+    virtual void updateKeyboardMapping(const std::function<void(keyboard_to_virtual_events_t&)>& mapKeys) = 0;
+    virtual void updateMouseMapping(const std::function<void(mouse_to_virtual_events_t&)>& mapKeys) = 0;
+    virtual void updateImguizmoMapping(const std::function<void(imguizmo_to_virtual_events_t&)>& mapKeys) = 0;


I don't get the std::functions, this is virtual, but the implementation just calls the function?!
and calling the function is simply a lambda. this is super unnecessary, the members are public.

why would you want to modify them through a std::function? just access the member, assign it, clear it.

I'd get rid of the whole updateXXXMapping through function and modify the data directly

Erfan-Ahmadi · 2026-09-04T11:35:49Z

+        /// @brief Virtual event emitted by this binding.
+        gimbal_event_t event = {};
+        /// @brief Per-binding gain applied when raw input is converted into one virtual-event magnitude.
+        double magnitudeScale = DefaultMagnitudeScale;


float is enough

Erfan-Ahmadi · 2026-09-04T11:53:16Z

+        /// @brief Per-binding gain applied when raw input is converted into one virtual-event magnitude.
+        double magnitudeScale = DefaultMagnitudeScale;
+        /// @brief Runtime latch used by held keyboard and mouse-button bindings.
+        bool active = false;


CHashInfo is two unrelated things in one struct. same for magnitude inside the event

get rid of sanitize

get rid of useless std::functions accessors that is equivalent to returning map&

then I would look for a solution like this:

struct SBinding { // pure config — what lives in every stored layout uint8_t eventIndex; // index into CVirtualGimbalEvent::VirtualEventsTypeTable float magnitudeScale; // sensitivity; float is ample for a multiplier }; // = 8 bytes

Binding layout would just become a struct holding three maps and that's it ;)

State of active could be:

std::bitset<EKC_COUNT> m_heldKeys; // 16 B std::bitset<EMC_COUNT> m_heldButtons; // 8 B std::array<double, CVirtualGimbalEvent::EventsCount> m_magnitude; // 144 B // = 168 B, fixed

also active+magnitude should not be something that "InputBinding" should care. that's why I suggest moving it up the hierarchy when used in IGimbalInputProcessor

this will not only be a net reduction in memory usage, but cleans things up a lot better. and you have data with different frequncies living seperately and in a better place

Erfan-Ahmadi · 2026-09-04T11:54:17Z

+/// Relative mouse movement, mouse scroll, and ImGuizmo deltas emit
+/// `abs(rawDelta) * magnitudeScale` per bound axis. The result is written into
+/// `CVirtualGimbalEvent::magnitude`.
+class IGimbalInputProcessor : public CGimbalBindingLayoutStorage


InputProcessor needs to store map of the bindings from input to events.
and it makes 0 sense that a "processor" inherits from a "BindingLayout"

you already needed it somewhere else and decided to store it as member function.
but for some reason this inherits?

// in PlanarProjection ui::CGimbalBindingLayoutStorage m_inputBinding

based on https://github.com/Devsh-Graphics-Programming/Nabla/pull/995/changes#r3933703955 this inheritance should be changed to just storage of the static "layout" (the maps) + dynamic "states" (the active status + magnitude)
also interface inheriting from impl was a bit sketchy

Erfan-Ahmadi · 2026-09-07T09:26:58Z

+    using inv_concatenated_matrix_t = std::optional<hlsl::float64_t4x4>;
+
+    /// @brief One concrete linear projection matrix together with cached inverse metadata.
+    struct CProjection : public IProjection


I don't get the choice to have ILinearProjection::CProjection inherit from IProjection and not the ILinearProjection itself
I would assume the convention IXXXProjection inherits from IProjection.

Erfan-Ahmadi · 2026-09-07T10:25:28Z

+/// `MinDistance` prevents zero-distance target-relative states.
+/// `DefaultMaxDistance` is unbounded. Individual cameras and tools may apply
+/// their own finite limits on top of it.
+struct SCameraTargetRelativeTraits final


"Traits" is usually something that get specialized via template on different classes
this header looks more like a "limit" to me.
naming convention aside.

why do we need to hardcode clamp distance to target by 0.1f?

why are values here float and the ones in SCameraToolingThresholds below in double?

I think this MinDistance value should be the same as whatever the projection paired with the camera uses as zNear plane.

but again, I don't get the reason of existance for MinDistance and why we need to clamp some distance between a costexpr Min and Max that's baked into the library

Erfan-Ahmadi · 2026-09-07T10:48:57Z

+    /// the minimum representable `float` value.
+    static inline constexpr float MinDistance = 1e-1f;
+    /// @brief Default upper bound for target-relative distance when no camera-specific cap is requested.
+    static inline constexpr float DefaultMaxDistance = std::numeric_limits<float>::infinity();


again, this is float, and from what I've seen you're clamping/comparing doubles with this.
float infinity is not double infinity

Erfan-Ahmadi · 2026-09-07T11:00:04Z

+};
+
+/// @brief Comparison thresholds used by helper layers outside the runtime camera interface.
+struct SCameraToolingThresholds final


the limits in these files need to clarify:

Numerical guards — "will this divide by zero?"

Policy floors — "should we let the user do this?" A UX decision

e.g. MinDistance, which I think is useless why would we need to clamp a positive number to 0.1 and +inf? it's not guarding a div and it seems to be soley UX related.

Comparison tolerances — "are these two states the same?" and the epsilons relate to the floating point representation used. and their values need to be justified, their usages need to be justified as well. what are we protecting against?

Erfan-Ahmadi · 2026-09-07T11:16:25Z

+// This file is part of the "Nabla Engine".
+// For conditions of distribution and use, see copyright notice in nabla.h
+
+#ifndef _C_CAMERA_TRAITS_HPP_


CCameraMathUtils has similar limits structs, maybe they should live in the same place.

Erfan-Ahmadi · 2026-09-07T11:41:58Z

+    }
+
+    template<typename Vec, typename E = double>
+    static inline bool isOrthoBase(const Vec& x, const Vec& y, const Vec& z, const E epsilon = 1e-6)


the problem here is that's it's hardcoded for fp32 but templated on float type. you could get away with 1e-13 for doubles here
also, instead of "length" which has a sqrt involved. you could just abs(dot(V,V)-1.0) < eps

Erfan-Ahmadi · 2026-09-07T12:30:07Z

+        hlsl::matrix<precision_t, 3, 3> m_orthonormal;
+
+        /// @brief Counter that increments for each performed manipulation, resets with each begin() call
+        size_t m_counter = {};


why reset every frame?
Isn't the point of the counter to avoid doing things everyframe and the manipulation to persist across frames? it's stored as uint64_t/size_t which means every frame there can be 2^64-1 manipulations?! even uint16 seems enough to me (~65K) for every frame.

Oh no! you're just doing &=bool(m_gimbal.getManipulationCounter()); everywhere, it is literally just 1 bit/bool needed but somehow we have counter and isManipulating, both are useless and replacable by a bool at this stage.

I'm going to go an step further, remove the begin/end and counter reset everyframe and the weird bool below. store a 64-bit counter and increment on every edit. higher level code could store a gimbal manipulation counter value next to their computed view matrix and recalc when counters mismatch.
also some functions appear to be missing manipulation_counter++ like setScale

Erfan-Ahmadi · 2026-09-07T12:30:16Z

+        size_t m_counter = {};
+
+        /// @brief Tracks whether gimbal is currently in manipulation mode
+        bool m_isManipulating = false;


this is just for assert(m_isManipulating) it seems. need to remove

Erfan-Ahmadi · 2026-09-08T10:46:38Z

+                if constexpr (AllowedEvents & CVirtualGimbalEvent::ScaleXInc)
+                    if (event.type == CVirtualGimbalEvent::ScaleXInc)
+                        impulse.dVirtualScale.x *= static_cast<precision_t>(event.magnitude);
+
+                if constexpr (AllowedEvents & CVirtualGimbalEvent::ScaleXDec)
+                    if (event.type == CVirtualGimbalEvent::ScaleXDec)
+                        impulse.dVirtualScale.x *= static_cast<precision_t>(event.magnitude);


you don't need scale for camera gimbal

this class is not supposed to be general model matrix construction that handles scale, this class is all about cameras and virtual events affecting cameras.

also I looked and there is literally 0 difference between CVirtualGimbalEvent::ScaleXInc and CVirtualGimbalEvent::ScaleXDec. you're treating them as the same thing everywhere

also no camera allows scaling gimball event as it makes 0 sense

we need to nuke the scale event completely and remove m_scale from here.

Erfan-Ahmadi · 2026-09-08T10:50:04Z

+    template<typename T>
+    static inline constexpr camera_vector_t<T, 3> getCameraWorldRight()
+    {
+        return camera_vector_t<T, 3>(T(1), T(0), T(0));
+    }
+
+    template<typename T>
+    static inline constexpr camera_vector_t<T, 3> getCameraWorldUp()
+    {
+        return camera_vector_t<T, 3>(T(0), T(1), T(0));
+    }
+
+    template<typename T>
+    static inline constexpr camera_vector_t<T, 3> getCameraWorldForward()
+    {
+        return camera_vector_t<T, 3>(T(0), T(0), T(1));
+    }


very bad names and types.
In linear algebra it's just simply standard basis X Y Z, has nothing to do with the camera.
you transform these vectors using quaternions to put into matrix columns and construct a matrix that transforms these standard basis to the new ones.

need to rename to something that makes more sense or get rid of the functions alltogether. writing it directly makes more sense and is more readable

Erfan-Ahmadi · 2026-09-08T11:47:38Z

+    /// The class exists mainly as a convenient instantiable type when no additional
+    /// camera-specific state or manipulation policy is required on top of `IGimbal`.
+    template<typename T = hlsl::float64_t>
+    class CGeneralPurposeGimbal : public IGimbal<T>


unused class and adds 0 value to parent class

Erfan-Ahmadi · 2026-09-08T11:50:08Z

+        }
+
+        /// @brief Apply a prebuilt rigid reference transform and an accumulated impulse in one step.
+        inline void transform(const CReferenceTransform& reference, const VirtualImpulse& impulse)


this function is not used anywhere. just keeping notes for future

Erfan-Ahmadi · 2026-09-08T11:57:36Z

+        if constexpr (std::is_same_v<T, float>)
+            return makeQuaternionFromBasisImpl(canonicalRight, canonicalUp, canonicalForward);
+        else
+            return makeQuaternionFromBasisImpl(canonicalRight, canonicalUp, canonicalForward);


these are the exact same, no need to specialize with if constexpr

Erfan-Ahmadi · 2026-09-08T11:58:03Z

+        {
+            setOrientation(reference.orientation * hlsl::CCameraMathUtilities::makeQuaternionFromEulerRadiansYXZ(impulse.dVirtualRotation));
+            setPosition(
+                hlsl::float64_t3(reference.frame[3]) +


templated on T but using float64_t3 directly here?

doesn't matter, this will all change to float64 soon

Erfan-Ahmadi · 2026-09-08T12:11:21Z

+            if(dRadians)
+                m_counter++;
+
+            const auto dRotation = hlsl::CCameraMathUtilities::makeQuaternionFromAxisAngle(axis, static_cast<precision_t>(dRadians));


float for radians/angle is enough precision. so this is fine.
although that function needs to go into actual hlsl lib

Erfan-Ahmadi · 2026-09-08T12:25:46Z

+
+        /// @brief Accumulates one frame of virtual events into a translation/rotation/scale impulse.
+        template <uint32_t AllowedEvents>
+        VirtualImpulse accumulate(std::span<const CVirtualGimbalEvent> virtualEvents, const vector_t<3u>& gRightOverride, const vector_t<3u>& gUpOverride, const vector_t<3u>& gForwardOverride)


3 params are not used at all.

Erfan-Ahmadi · 2026-09-08T12:33:05Z

+            m_viewMatrix[0u] = hlsl::float64_t4(gRight, -hlsl::dot(gRight, position));
+            m_viewMatrix[1u] = hlsl::float64_t4(gUp, -hlsl::dot(gUp, position));
+            m_viewMatrix[2u] = hlsl::float64_t4(gForward, -hlsl::dot(gForward, position));


make sure to mention that the inverse of an orthonormal matrix is it's transpose, so we caan put columns into rows.
also it's really confusing because you're treating m_orthonormal (where those getX/Y/ZAxis come from) as a column-major order matrix treating mat[0] as the first column but here you're doing it properly with nabla conventions (hlsl::matrix is row matrix)

and the conventions aren't clear, the when I look at some functions, some treat it as column-major some as row-major

Erfan-Ahmadi · 2026-09-08T12:59:12Z

+    template<typename T>
+    static inline camera_vector_t<T, 3> projectWorldVectorToLocalBasis(
+        const camera_vector_t<T, 3>& worldVector,
+        const camera_vector_t<T, 3>& right,
+        const camera_vector_t<T, 3>& up,
+        const camera_vector_t<T, 3>& forward)
+    {
+        const camera_matrix_t<T, 3, 3> basis { right, up, forward };
+        return hlsl::mul(hlsl::transpose(basis), worldVector);
+    }
+
+    template<typename T>
+    static inline camera_vector_t<T, 3> transformLocalVectorToWorldBasis(
+        const camera_vector_t<T, 3>& localVector,
+        const camera_vector_t<T, 3>& right,
+        const camera_vector_t<T, 3>& up,
+        const camera_vector_t<T, 3>& forward)
+    {
+        const camera_matrix_t<T, 3, 3> basis { right, up, forward };
+        return hlsl::mul(basis, localVector);
+    }


this is wrong and swapped because hlsl::matrix and mul treats matrix as row major matrix.
I'm 90% sure this is causing a bug with the CSphericalTargetCamera::applyPlanarTargetTranslation using this function

AnastaZIuk added 30 commits October 25, 2024 08:38

add operator= for glm:: matrices corresponding to our at home matrice…

cdf867d

…s - we were never be able to do ourMatrixT = glmMatrixT and its important since a lot of utils create glmMatrixT actually

I'm stupid, I have explicit at home matrix constructor taking glm:: m…

6ef4f89

…atrix xD remove the = operator glm -> to ours

steal include/nbl/builtin/hlsl/matrix_utils/transformation_matrix_uti…

22f505c

…ls.hlsl from #760

fix getMatrix3x4As4x4 (return type issues) & buildCameraLookAtMatrixR…

51f4ced

…H (ambiguity dependent type issues), reference #760

update examples_tests submodule

7f2c085

actually *this* addresses https://github.com/Devsh-Graphics-Programmi…

f1daa25

…ng/Nabla/pull/760/files#r1816728485 for #760 PR, update examples_tests submodule

update examples_tests submodule

2192e99

update examples_tests submodule

520b1f5

forgot to commit projection build methods, update examples_tests subm…

a0890f0

…odule

add is_smart_refctd_ptr_v, update examples_tests submodule

0109b22

update examples_tests submodule

2a29a2f

update examples_tests submodule

7673769

pull master & resolve conflicts

fc7bc33

pull mastrer & resolve conflicts

8fc4f89

update examples_tests submodule

9a139df

create ui::E_MOUSE_CODE & constexpr ui::mouseCodeToString - we really…

24a80c9

… were missing an equivalent of E_KEY_CODE. Update examples_tests submodule

pull master & resolve conflicts

03fa2ca

@alichraghi small homework - read https://en.cppreference.com/w/cpp/l…

21b89b4

…anguage/inline and afterwards https://gudok.xyz/inline/ please

update examples_tests submodule

68bc7be

update examples_tests submodule

866e844

update examples_tests submodule

8bd6436

update examples_tests submodule

de69afe

update imguizmo with its upstream

a55bd39

update examples_tests submodule

6f3c5fc

update examples_tests submodule

0d126df

update examples_tests submodule

31e85e7

update examples_tests submodule

08f11f6

add getCastedMatrix & getCastedVector, update examples_tests submodule

5bcd6ef

update examples_tests submodule

5a07fe7

update examples_tests submodule

f55b268

Erfan-Ahmadi reviewed Sep 4, 2026

View reviewed changes

Erfan-Ahmadi reviewed Sep 7, 2026

View reviewed changes

Erfan-Ahmadi reviewed Sep 8, 2026

View reviewed changes

Uh oh!

Conversation

AnastaZIuk commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Erfan-Ahmadi Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Erfan-Ahmadi Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Erfan-Ahmadi Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

AnastaZIuk commented Feb 2, 2026 •

edited

Loading

Erfan-Ahmadi Sep 4, 2026 •

edited

Loading

Erfan-Ahmadi Sep 4, 2026 •

edited

Loading

Erfan-Ahmadi Sep 7, 2026 •

edited

Loading