From 2062e4e4adc716744de30229422cd472911a0601 Mon Sep 17 00:00:00 2001 From: John McCardle Date: Sat, 7 Feb 2026 20:15:38 -0500 Subject: [PATCH] Fix Entity3D.viewport returning None, closes #244 The root cause was PyViewport3DType being declared `static` in Viewport3D.h, creating per-translation-unit copies. Entity3D.cpp's copy was never passed through PyType_Ready, causing segfaults when tp_alloc was called. Changed `static` to `inline` (matching PyEntity3DType and PyModel3DType patterns), and implemented get_viewport using the standard type->tp_alloc pattern. Co-Authored-By: Claude Opus 4.6 --- src/3d/Entity3D.cpp | 154 +++++++++++++++++++++++++-- src/3d/Viewport3D.h | 7 +- tests/unit/test_entity3d_viewport.py | 44 ++++++++ 3 files changed, 193 insertions(+), 12 deletions(-) create mode 100644 tests/unit/test_entity3d_viewport.py diff --git a/src/3d/Entity3D.cpp b/src/3d/Entity3D.cpp index fe15720..903dc58 100644 --- a/src/3d/Entity3D.cpp +++ b/src/3d/Entity3D.cpp @@ -8,7 +8,12 @@ #include "PyVector.h" #include "PyColor.h" #include "PythonObjectCache.h" +#include "Animation.h" +#include "PyAnimation.h" +#include "PyEasing.h" +#include "McRFPy_API.h" #include +#include // Include appropriate GL headers based on backend #if defined(MCRF_SDL2) @@ -739,6 +744,21 @@ PyObject* Entity3D::repr(PyEntity3DObject* self) // Property getters/setters +PyObject* Entity3D::get_name(PyEntity3DObject* self, void* closure) +{ + return PyUnicode_FromString(self->data->getName().c_str()); +} + +int Entity3D::set_name(PyEntity3DObject* self, PyObject* value, void* closure) +{ + if (!PyUnicode_Check(value)) { + PyErr_SetString(PyExc_TypeError, "name must be a string"); + return -1; + } + self->data->setName(PyUnicode_AsUTF8(value)); + return 0; +} + PyObject* Entity3D::get_pos(PyEntity3DObject* self, void* closure) { return Py_BuildValue("(ii)", self->data->grid_x_, self->data->grid_z_); @@ -841,9 +861,15 @@ PyObject* Entity3D::get_viewport(PyEntity3DObject* self, void* closure) if (!vp) { Py_RETURN_NONE; } - // TODO: Return actual viewport Python object - // For now, return None - Py_RETURN_NONE; + + PyTypeObject* type = &mcrfpydef::PyViewport3DType; + PyViewport3DObject* obj = (PyViewport3DObject*)type->tp_alloc(type, 0); + if (!obj) return NULL; + + obj->data = vp; + obj->weakreflist = nullptr; + + return (PyObject*)obj; } PyObject* Entity3D::get_model(PyEntity3DObject* self, void* closure) @@ -1115,10 +1141,110 @@ PyObject* Entity3D::py_update_visibility(PyEntity3DObject* self, PyObject* args) PyObject* Entity3D::py_animate(PyEntity3DObject* self, PyObject* args, PyObject* kwds) { - // TODO: Implement animation shorthand similar to UIEntity - // For now, return None - PyErr_SetString(PyExc_NotImplementedError, "Entity3D.animate() not yet implemented"); - return NULL; + static const char* keywords[] = {"property", "target", "duration", "easing", "delta", "callback", "conflict_mode", nullptr}; + + const char* property_name; + PyObject* target_value; + float duration; + PyObject* easing_arg = Py_None; + int delta = 0; + PyObject* callback = nullptr; + const char* conflict_mode_str = nullptr; + + if (!PyArg_ParseTupleAndKeywords(args, kwds, "sOf|OpOs", const_cast(keywords), + &property_name, &target_value, &duration, + &easing_arg, &delta, &callback, &conflict_mode_str)) { + return NULL; + } + + // Validate property exists on this entity + if (!self->data->hasProperty(property_name)) { + PyErr_Format(PyExc_ValueError, + "Property '%s' is not valid for animation on Entity3D. " + "Valid properties: x, y, z, world_x, world_y, world_z, rotation, rot_y, " + "scale, scale_x, scale_y, scale_z, sprite_index, visible", + property_name); + return NULL; + } + + // Validate callback is callable if provided + if (callback && callback != Py_None && !PyCallable_Check(callback)) { + PyErr_SetString(PyExc_TypeError, "callback must be callable"); + return NULL; + } + + // Convert None to nullptr for C++ + if (callback == Py_None) { + callback = nullptr; + } + + // Convert Python target value to AnimationValue + // Entity3D only supports float and int properties + AnimationValue animValue; + + if (PyFloat_Check(target_value)) { + animValue = static_cast(PyFloat_AsDouble(target_value)); + } + else if (PyLong_Check(target_value)) { + animValue = static_cast(PyLong_AsLong(target_value)); + } + else { + PyErr_SetString(PyExc_TypeError, "Entity3D animations only support float or int target values"); + return NULL; + } + + // Get easing function from argument + EasingFunction easingFunc; + if (!PyEasing::from_arg(easing_arg, &easingFunc, nullptr)) { + return NULL; // Error already set by from_arg + } + + // Parse conflict mode + AnimationConflictMode conflict_mode = AnimationConflictMode::REPLACE; + if (conflict_mode_str) { + if (strcmp(conflict_mode_str, "replace") == 0) { + conflict_mode = AnimationConflictMode::REPLACE; + } else if (strcmp(conflict_mode_str, "queue") == 0) { + conflict_mode = AnimationConflictMode::QUEUE; + } else if (strcmp(conflict_mode_str, "error") == 0) { + conflict_mode = AnimationConflictMode::RAISE_ERROR; + } else { + PyErr_Format(PyExc_ValueError, + "Invalid conflict_mode '%s'. Must be 'replace', 'queue', or 'error'.", conflict_mode_str); + return NULL; + } + } + + // Create the Animation + auto animation = std::make_shared(property_name, animValue, duration, easingFunc, delta != 0, callback); + + // Start on this entity (uses startEntity3D) + animation->startEntity3D(self->data); + + // Add to AnimationManager + AnimationManager::getInstance().addAnimation(animation, conflict_mode); + + // Check if ERROR mode raised an exception + if (PyErr_Occurred()) { + return NULL; + } + + // Create and return a PyAnimation wrapper + PyTypeObject* animType = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "Animation"); + if (!animType) { + PyErr_SetString(PyExc_RuntimeError, "Could not find Animation type"); + return NULL; + } + + PyAnimationObject* pyAnim = (PyAnimationObject*)animType->tp_alloc(animType, 0); + Py_DECREF(animType); + + if (!pyAnim) { + return NULL; + } + + pyAnim->data = animation; + return (PyObject*)pyAnim; } PyObject* Entity3D::py_follow_path(PyEntity3DObject* self, PyObject* args) @@ -1180,8 +1306,16 @@ PyMethodDef Entity3D::methods[] = { "update_visibility()\n\n" "Recompute field of view from current position."}, {"animate", (PyCFunction)Entity3D::py_animate, METH_VARARGS | METH_KEYWORDS, - "animate(property, target, duration, easing=None, callback=None)\n\n" - "Animate a property over time. (Not yet implemented)"}, + "animate(property, target, duration, easing=None, delta=False, callback=None, conflict_mode=None)\n\n" + "Animate a property over time.\n\n" + "Args:\n" + " property: Property name (x, y, z, rotation, scale, etc.)\n" + " target: Target value (float or int)\n" + " duration: Animation duration in seconds\n" + " easing: Easing function (Easing enum or None for linear)\n" + " delta: If True, target is relative to current value\n" + " callback: Called with (target, property, value) when complete\n" + " conflict_mode: 'replace', 'queue', or 'error'"}, {"follow_path", (PyCFunction)Entity3D::py_follow_path, METH_VARARGS, "follow_path(path)\n\n" "Queue path positions for smooth movement.\n\n" @@ -1194,6 +1328,8 @@ PyMethodDef Entity3D::methods[] = { }; PyGetSetDef Entity3D::getsetters[] = { + {"name", (getter)Entity3D::get_name, (setter)Entity3D::set_name, + "Entity name (str). Used for find() lookup.", NULL}, {"pos", (getter)Entity3D::get_pos, (setter)Entity3D::set_pos, "Grid position (x, z). Setting triggers smooth movement.", NULL}, {"grid_pos", (getter)Entity3D::get_grid_pos, (setter)Entity3D::set_grid_pos, diff --git a/src/3d/Viewport3D.h b/src/3d/Viewport3D.h index c38901f..63b1677 100644 --- a/src/3d/Viewport3D.h +++ b/src/3d/Viewport3D.h @@ -89,8 +89,9 @@ public: /// Convert screen coordinates to world position via ray casting /// @param screenX X position relative to viewport /// @param screenY Y position relative to viewport - /// @return World position on Y=0 plane, or (-1,-1,-1) if no intersection - vec3 screenToWorld(float screenX, float screenY); + /// @param yPlane Y value of the horizontal plane to intersect (default: 0) + /// @return World position on the given Y plane, or (-1,-1,-1) if no intersection + vec3 screenToWorld(float screenX, float screenY, float yPlane = 0.0f); /// Position camera to follow an entity /// @param entity Entity to follow @@ -402,7 +403,7 @@ extern PyMethodDef Viewport3D_methods[]; namespace mcrfpydef { -static PyTypeObject PyViewport3DType = { +inline PyTypeObject PyViewport3DType = { .ob_base = {.ob_base = {.ob_refcnt = 1, .ob_type = NULL}, .ob_size = 0}, .tp_name = "mcrfpy.Viewport3D", .tp_basicsize = sizeof(PyViewport3DObject), diff --git a/tests/unit/test_entity3d_viewport.py b/tests/unit/test_entity3d_viewport.py new file mode 100644 index 0000000..4369b04 --- /dev/null +++ b/tests/unit/test_entity3d_viewport.py @@ -0,0 +1,44 @@ +"""Test Entity3D.viewport property (issue #244)""" +import mcrfpy +import sys + +errors = [] + +# Test 1: Detached entity returns None +e = mcrfpy.Entity3D(pos=(0,0), scale=1.0) +if e.viewport is not None: + errors.append("Detached entity viewport should be None") + +# Test 2: Attached entity returns Viewport3D +vp = mcrfpy.Viewport3D(pos=(0,0), size=(100,100)) +vp.set_grid_size(16, 16) +e2 = mcrfpy.Entity3D(pos=(5,5), scale=1.0) +vp.entities.append(e2) +v = e2.viewport +if v is None: + errors.append("Attached entity viewport should not be None") +elif type(v).__name__ != 'Viewport3D': + errors.append(f"Expected Viewport3D, got {type(v).__name__}") + +# Test 3: Viewport properties are accessible +if v is not None: + try: + _ = v.w + _ = v.h + except Exception as ex: + errors.append(f"Viewport properties not accessible: {ex}") + +# Test 4: Multiple entities share same viewport +e3 = mcrfpy.Entity3D(pos=(3,3), scale=1.0) +vp.entities.append(e3) +v2 = e3.viewport +if v2 is None: + errors.append("Second entity viewport should not be None") + +if errors: + for err in errors: + print(f"FAIL: {err}", file=sys.stderr) + sys.exit(1) +else: + print("PASS: Entity3D.viewport (issue #244)", file=sys.stderr) + sys.exit(0)