From 77fea553de9e2d9154d3098de29d671d2f14f5d2 Mon Sep 17 00:00:00 2001 From: Cory Petkovsek <632766+tinmanjuggernaut@users.noreply.github.com> Date: Sat, 6 Nov 2021 06:55:15 +0800 Subject: [PATCH 1/2] RIDs enforce clearing dangling pointers, except in Script --- core/rid.h | 65 ++++++------------- doc/classes/PhysicsServer.xml | 12 ++++ doc/classes/VisualServer.xml | 12 ++++ drivers/gles2/rasterizer_scene_gles2.cpp | 2 +- drivers/gles2/rasterizer_scene_gles2.h | 2 +- drivers/gles2/rasterizer_storage_gles2.cpp | 2 +- drivers/gles2/rasterizer_storage_gles2.h | 2 +- drivers/gles3/rasterizer_scene_gles3.cpp | 2 +- drivers/gles3/rasterizer_scene_gles3.h | 2 +- drivers/gles3/rasterizer_storage_gles3.cpp | 2 +- drivers/gles3/rasterizer_storage_gles3.h | 2 +- .../animation_player_editor_plugin.cpp | 2 +- editor/plugins/spatial_editor_plugin.cpp | 13 +++- editor/spatial_editor_gizmos.cpp | 4 +- modules/bullet/bullet_physics_server.cpp | 8 ++- modules/bullet/bullet_physics_server.h | 2 +- modules/gridmap/grid_map.cpp | 10 +-- servers/physics/physics_server_sw.cpp | 8 ++- servers/physics/physics_server_sw.h | 2 +- servers/physics_2d_server.cpp | 2 +- servers/physics_2d_server.h | 1 + servers/physics_server.cpp | 2 +- servers/physics_server.h | 3 +- servers/visual/rasterizer.h | 4 +- servers/visual/visual_server_canvas.cpp | 2 +- servers/visual/visual_server_canvas.h | 2 +- servers/visual/visual_server_raster.cpp | 7 +- servers/visual/visual_server_raster.h | 2 +- servers/visual/visual_server_scene.cpp | 10 ++- servers/visual/visual_server_scene.h | 2 +- servers/visual/visual_server_viewport.cpp | 2 +- servers/visual/visual_server_viewport.h | 2 +- servers/visual/visual_server_wrap_mt.h | 2 +- servers/visual_server.cpp | 2 +- servers/visual_server.h | 4 +- 35 files changed, 113 insertions(+), 90 deletions(-) diff --git a/core/rid.h b/core/rid.h index fd7e4b5687..5a58253ea5 100644 --- a/core/rid.h +++ b/core/rid.h @@ -42,9 +42,7 @@ class RID_OwnerBase; class RID_Data { friend class RID_OwnerBase; -#ifndef DEBUG_ENABLED RID_OwnerBase *_owner; -#endif uint32_t _id; public: @@ -61,6 +59,8 @@ class RID { public: _FORCE_INLINE_ RID_Data *get_data() const { return _data; } + //_FORCE_INLINE_ void erase(RID &p_rid) { _data = nullptr; } // Useful to have? + _FORCE_INLINE_ bool operator==(const RID &p_rid) const { return _data == p_rid._data; } @@ -78,7 +78,16 @@ public: } _FORCE_INLINE_ bool is_valid() const { return _data != nullptr; } - _FORCE_INLINE_ uint32_t get_id() const { return _data ? _data->get_id() : 0; } + _FORCE_INLINE_ uint32_t get_id() const { + if (_data) { + try { + return _data->get_id(); + } catch (...) { + ERR_FAIL_V_MSG(0, "RID points to memory that has already been freed."); + } + } + return 0; // An invalid or uninitialized state + } _FORCE_INLINE_ RID() { _data = nullptr; @@ -92,63 +101,41 @@ protected: p_rid._data = p_data; refcount.ref(); p_data->_id = refcount.get(); -#ifndef DEBUG_ENABLED p_data->_owner = this; -#endif } -#ifndef DEBUG_ENABLED + _FORCE_INLINE_ void _free(RID &p_rid) { + p_rid._data = nullptr; + } _FORCE_INLINE_ bool _is_owner(const RID &p_rid) const { return this == p_rid._data->_owner; } _FORCE_INLINE_ void _remove_owner(RID &p_rid) { - p_rid._data->_owner = NULL; + p_rid._data->_owner = nullptr; } -#endif public: virtual void get_owned_list(List *p_owned) = 0; - static void init_rid(); virtual ~RID_OwnerBase() {} }; template class RID_Owner : public RID_OwnerBase { -public: -#ifdef DEBUG_ENABLED - mutable Set id_map; -#endif public: _FORCE_INLINE_ RID make_rid(T *p_data) { RID rid; _set_data(rid, p_data); - -#ifdef DEBUG_ENABLED - id_map.insert(p_data); -#endif - return rid; } _FORCE_INLINE_ T *get(const RID &p_rid) { -#ifdef DEBUG_ENABLED - - ERR_FAIL_COND_V(!p_rid.is_valid(), nullptr); - ERR_FAIL_COND_V(!id_map.has(p_rid.get_data()), nullptr); -#endif return static_cast(p_rid.get_data()); } _FORCE_INLINE_ T *getornull(const RID &p_rid) { -#ifdef DEBUG_ENABLED - - if (p_rid.get_data()) { - ERR_FAIL_COND_V(!id_map.has(p_rid.get_data()), nullptr); - } -#endif return static_cast(p_rid.get_data()); } @@ -160,30 +147,16 @@ public: if (p_rid.get_data() == nullptr) { return false; } -#ifdef DEBUG_ENABLED - return id_map.has(p_rid.get_data()); -#else return _is_owner(p_rid); -#endif } - void free(RID p_rid) { -#ifdef DEBUG_ENABLED - id_map.erase(p_rid.get_data()); -#else + void free(RID &p_rid) { _remove_owner(p_rid); -#endif + _free(p_rid); } void get_owned_list(List *p_owned) { -#ifdef DEBUG_ENABLED - - for (typename Set::Element *E = id_map.front(); E; E = E->next()) { - RID r; - _set_data(r, static_cast(E->get())); - p_owned->push_back(r); - } -#endif + // Used to be a debug only function disabled during release mode. Now deprecated. } }; diff --git a/doc/classes/PhysicsServer.xml b/doc/classes/PhysicsServer.xml index 107a874122..034a45effd 100644 --- a/doc/classes/PhysicsServer.xml +++ b/doc/classes/PhysicsServer.xml @@ -630,6 +630,18 @@ Destroys any of the objects created by PhysicsServer. If the [RID] passed is not one of the objects that can be created by PhysicsServer, an error will be sent to the console. + Note: After freeing the object, the RID now has a reference to invalid memory. It is not safe to free an invalid RID. Make sure to reset it before using it again by assigning it to `RID()`. + [code] + var r:RID = PhysicsServer.space_create() + PhysicsServer.free_rid(r) + print("ID: ", r.get_id()) # Not safe to access or free an invalid RID + r = RID() # Reset RID, safe to use or free again + print("ID: ", r.get_id()) + + Output: + ID: 157 + ID: 0 + [/code] diff --git a/doc/classes/VisualServer.xml b/doc/classes/VisualServer.xml index f88059ac18..25f61a4572 100644 --- a/doc/classes/VisualServer.xml +++ b/doc/classes/VisualServer.xml @@ -962,6 +962,18 @@ Tries to free an object in the VisualServer. + Note: After freeing the object, the RID now has a reference to invalid memory. It is not safe to free an invalid RID. Make sure to reset it before using it again by assigning it to `RID()`. + [code] + var r:RID = VisualServer.get_test_cube() + VisualServer.free_rid(r) + print("ID: ", r.get_id()) # Not safe to access or free an invalid RID + r = RID() # Reset RID, safe to use or free again + print("ID: ", r.get_id()) + + Output: + ID: 157 + ID: 0 + [/code] diff --git a/drivers/gles2/rasterizer_scene_gles2.cpp b/drivers/gles2/rasterizer_scene_gles2.cpp index 31362045e2..ec2aea7b38 100644 --- a/drivers/gles2/rasterizer_scene_gles2.cpp +++ b/drivers/gles2/rasterizer_scene_gles2.cpp @@ -3830,7 +3830,7 @@ void RasterizerSceneGLES2::set_scene_pass(uint64_t p_pass) { scene_pass = p_pass; } -bool RasterizerSceneGLES2::free(RID p_rid) { +bool RasterizerSceneGLES2::free(RID &p_rid) { if (light_instance_owner.owns(p_rid)) { LightInstance *light_instance = light_instance_owner.getptr(p_rid); diff --git a/drivers/gles2/rasterizer_scene_gles2.h b/drivers/gles2/rasterizer_scene_gles2.h index 4df57d7270..2214f0333d 100644 --- a/drivers/gles2/rasterizer_scene_gles2.h +++ b/drivers/gles2/rasterizer_scene_gles2.h @@ -764,7 +764,7 @@ public: virtual void render_scene(const Transform &p_cam_transform, const CameraMatrix &p_cam_projection, const int p_eye, bool p_cam_ortogonal, InstanceBase **p_cull_result, int p_cull_count, RID *p_light_cull_result, int p_light_cull_count, RID *p_reflection_probe_cull_result, int p_reflection_probe_cull_count, RID p_environment, RID p_shadow_atlas, RID p_reflection_atlas, RID p_reflection_probe, int p_reflection_probe_pass); virtual void render_shadow(RID p_light, RID p_shadow_atlas, int p_pass, InstanceBase **p_cull_result, int p_cull_count); - virtual bool free(RID p_rid); + virtual bool free(RID &p_rid); virtual void set_scene_pass(uint64_t p_pass); virtual void set_debug_draw_mode(VS::ViewportDebugDraw p_debug_draw); diff --git a/drivers/gles2/rasterizer_storage_gles2.cpp b/drivers/gles2/rasterizer_storage_gles2.cpp index 1a6efc83e2..f782b7f4da 100644 --- a/drivers/gles2/rasterizer_storage_gles2.cpp +++ b/drivers/gles2/rasterizer_storage_gles2.cpp @@ -5827,7 +5827,7 @@ VS::InstanceType RasterizerStorageGLES2::get_base_type(RID p_rid) const { } } -bool RasterizerStorageGLES2::free(RID p_rid) { +bool RasterizerStorageGLES2::free(RID &p_rid) { if (render_target_owner.owns(p_rid)) { RenderTarget *rt = render_target_owner.getornull(p_rid); _render_target_clear(rt); diff --git a/drivers/gles2/rasterizer_storage_gles2.h b/drivers/gles2/rasterizer_storage_gles2.h index e469b7da1c..a6eecb4cda 100644 --- a/drivers/gles2/rasterizer_storage_gles2.h +++ b/drivers/gles2/rasterizer_storage_gles2.h @@ -1324,7 +1324,7 @@ public: virtual VS::InstanceType get_base_type(RID p_rid) const; - virtual bool free(RID p_rid); + virtual bool free(RID &p_rid); struct Frame { RenderTarget *current_rt; diff --git a/drivers/gles3/rasterizer_scene_gles3.cpp b/drivers/gles3/rasterizer_scene_gles3.cpp index dfa25f045e..52881abe08 100644 --- a/drivers/gles3/rasterizer_scene_gles3.cpp +++ b/drivers/gles3/rasterizer_scene_gles3.cpp @@ -4849,7 +4849,7 @@ void RasterizerSceneGLES3::set_scene_pass(uint64_t p_pass) { scene_pass = p_pass; } -bool RasterizerSceneGLES3::free(RID p_rid) { +bool RasterizerSceneGLES3::free(RID &p_rid) { if (light_instance_owner.owns(p_rid)) { LightInstance *light_instance = light_instance_owner.getptr(p_rid); diff --git a/drivers/gles3/rasterizer_scene_gles3.h b/drivers/gles3/rasterizer_scene_gles3.h index d3a0fd09ba..6b7e978c12 100644 --- a/drivers/gles3/rasterizer_scene_gles3.h +++ b/drivers/gles3/rasterizer_scene_gles3.h @@ -855,7 +855,7 @@ public: bool _element_needs_directional_add(RenderList::Element *e); virtual void render_scene(const Transform &p_cam_transform, const CameraMatrix &p_cam_projection, const int p_eye, bool p_cam_ortogonal, InstanceBase **p_cull_result, int p_cull_count, RID *p_light_cull_result, int p_light_cull_count, RID *p_reflection_probe_cull_result, int p_reflection_probe_cull_count, RID p_environment, RID p_shadow_atlas, RID p_reflection_atlas, RID p_reflection_probe, int p_reflection_probe_pass); virtual void render_shadow(RID p_light, RID p_shadow_atlas, int p_pass, InstanceBase **p_cull_result, int p_cull_count); - virtual bool free(RID p_rid); + virtual bool free(RID &p_rid); virtual void set_scene_pass(uint64_t p_pass); virtual void set_debug_draw_mode(VS::ViewportDebugDraw p_debug_draw); diff --git a/drivers/gles3/rasterizer_storage_gles3.cpp b/drivers/gles3/rasterizer_storage_gles3.cpp index ad25504788..49452ff861 100644 --- a/drivers/gles3/rasterizer_storage_gles3.cpp +++ b/drivers/gles3/rasterizer_storage_gles3.cpp @@ -7727,7 +7727,7 @@ VS::InstanceType RasterizerStorageGLES3::get_base_type(RID p_rid) const { return VS::INSTANCE_NONE; } -bool RasterizerStorageGLES3::free(RID p_rid) { +bool RasterizerStorageGLES3::free(RID &p_rid) { if (render_target_owner.owns(p_rid)) { RenderTarget *rt = render_target_owner.getornull(p_rid); _render_target_clear(rt); diff --git a/drivers/gles3/rasterizer_storage_gles3.h b/drivers/gles3/rasterizer_storage_gles3.h index 934f293835..51006bfd29 100644 --- a/drivers/gles3/rasterizer_storage_gles3.h +++ b/drivers/gles3/rasterizer_storage_gles3.h @@ -1480,7 +1480,7 @@ public: virtual VS::InstanceType get_base_type(RID p_rid) const; - virtual bool free(RID p_rid); + virtual bool free(RID &p_rid); struct Frame { RenderTarget *current_rt; diff --git a/editor/plugins/animation_player_editor_plugin.cpp b/editor/plugins/animation_player_editor_plugin.cpp index c28ebd3ce8..f8e83ee80e 100644 --- a/editor/plugins/animation_player_editor_plugin.cpp +++ b/editor/plugins/animation_player_editor_plugin.cpp @@ -1292,7 +1292,7 @@ void AnimationPlayerEditor::_allocate_onion_layers() { void AnimationPlayerEditor::_free_onion_layers() { for (int i = 0; i < onion.captures.size(); i++) { if (onion.captures[i].is_valid()) { - VS::get_singleton()->free(onion.captures[i]); + VS::get_singleton()->free(onion.captures.get(i)); } } onion.captures.clear(); diff --git a/editor/plugins/spatial_editor_plugin.cpp b/editor/plugins/spatial_editor_plugin.cpp index 3ef78a7841..e3b79e1131 100644 --- a/editor/plugins/spatial_editor_plugin.cpp +++ b/editor/plugins/spatial_editor_plugin.cpp @@ -5860,9 +5860,16 @@ void SpatialEditor::_finish_indicators() { } void SpatialEditor::_finish_grid() { - for (int i = 0; i < 3; i++) { - VisualServer::get_singleton()->free(grid_instance[i]); - VisualServer::get_singleton()->free(grid[i]); + for (int a = 0; a < 3; a++) { + if(grid_enable[a]) { + int c = (a + 2) % 3; + if (grid_instance[c].is_valid() && grid_instance[c].get_id() > 0) { + VisualServer::get_singleton()->free(grid_instance[c]); + } + if (grid[c].is_valid() && grid[c].get_id() > 0) { + VisualServer::get_singleton()->free(grid[c]); + } + } } } diff --git a/editor/spatial_editor_gizmos.cpp b/editor/spatial_editor_gizmos.cpp index cf2f4c5e09..5ad9cced2b 100644 --- a/editor/spatial_editor_gizmos.cpp +++ b/editor/spatial_editor_gizmos.cpp @@ -90,7 +90,7 @@ bool EditorSpatialGizmo::is_editable() const { void EditorSpatialGizmo::clear() { for (int i = 0; i < instances.size(); i++) { if (instances[i].instance.is_valid()) { - VS::get_singleton()->free(instances[i].instance); + VS::get_singleton()->free(instances.get(i).instance); } } @@ -740,7 +740,7 @@ void EditorSpatialGizmo::free() { for (int i = 0; i < instances.size(); i++) { if (instances[i].instance.is_valid()) { - VS::get_singleton()->free(instances[i].instance); + VS::get_singleton()->free(instances.get(i).instance); } instances.write[i].instance = RID(); } diff --git a/modules/bullet/bullet_physics_server.cpp b/modules/bullet/bullet_physics_server.cpp index a4344665b5..e1f13e7af7 100644 --- a/modules/bullet/bullet_physics_server.cpp +++ b/modules/bullet/bullet_physics_server.cpp @@ -1474,7 +1474,11 @@ bool BulletPhysicsServer::generic_6dof_joint_get_flag(RID p_joint, Vector3::Axis return generic_6dof_joint->get_flag(p_axis, p_flag); } -void BulletPhysicsServer::free(RID p_rid) { +void BulletPhysicsServer::free(RID &p_rid) { + if (!p_rid.is_valid() || p_rid.get_id() == 0) { + return; + } + if (shape_owner.owns(p_rid)) { ShapeBullet *shape = shape_owner.get(p_rid); @@ -1528,7 +1532,7 @@ void BulletPhysicsServer::free(RID p_rid) { space_owner.free(p_rid); bulletdelete(space); } else { - ERR_FAIL_MSG("Invalid ID."); + ERR_FAIL_MSG("Invalid RID."); } } diff --git a/modules/bullet/bullet_physics_server.h b/modules/bullet/bullet_physics_server.h index 086097c385..fa1e1cf39a 100644 --- a/modules/bullet/bullet_physics_server.h +++ b/modules/bullet/bullet_physics_server.h @@ -377,7 +377,7 @@ public: /* MISC */ - virtual void free(RID p_rid); + virtual void free(RID &p_rid); virtual void set_active(bool p_active) { active = p_active; diff --git a/modules/gridmap/grid_map.cpp b/modules/gridmap/grid_map.cpp index 027906a5b8..c77d9d1b8a 100644 --- a/modules/gridmap/grid_map.cpp +++ b/modules/gridmap/grid_map.cpp @@ -416,8 +416,8 @@ bool GridMap::_octant_update(const OctantKey &p_key) { //erase multimeshes for (int i = 0; i < g.multimesh_instances.size(); i++) { - VS::get_singleton()->free(g.multimesh_instances[i].instance); - VS::get_singleton()->free(g.multimesh_instances[i].multimesh); + VS::get_singleton()->free(g.multimesh_instances.get(i).instance); + VS::get_singleton()->free(g.multimesh_instances.get(i).multimesh); } g.multimesh_instances.clear(); @@ -633,8 +633,8 @@ void GridMap::_octant_clean_up(const OctantKey &p_key) { //erase multimeshes for (int i = 0; i < g.multimesh_instances.size(); i++) { - VS::get_singleton()->free(g.multimesh_instances[i].instance); - VS::get_singleton()->free(g.multimesh_instances[i].multimesh); + VS::get_singleton()->free(g.multimesh_instances.get(i).instance); + VS::get_singleton()->free(g.multimesh_instances.get(i).multimesh); } g.multimesh_instances.clear(); } @@ -949,7 +949,7 @@ Vector3 GridMap::_get_offset() const { void GridMap::clear_baked_meshes() { for (int i = 0; i < baked_meshes.size(); i++) { - VS::get_singleton()->free(baked_meshes[i].instance); + VS::get_singleton()->free(baked_meshes.get(i).instance); } baked_meshes.clear(); diff --git a/servers/physics/physics_server_sw.cpp b/servers/physics/physics_server_sw.cpp index 365b98472e..2f3be882d8 100644 --- a/servers/physics/physics_server_sw.cpp +++ b/servers/physics/physics_server_sw.cpp @@ -1200,7 +1200,11 @@ bool PhysicsServerSW::generic_6dof_joint_get_flag(RID p_joint, Vector3::Axis p_a return generic_6dof_joint->get_flag(p_axis, p_flag); } -void PhysicsServerSW::free(RID p_rid) { +void PhysicsServerSW::free(RID &p_rid) { + if (!p_rid.is_valid() || p_rid.get_id() == 0) { + return; + } + _update_shapes(); //just in case if (shape_owner.owns(p_rid)) { @@ -1273,7 +1277,7 @@ void PhysicsServerSW::free(RID p_rid) { memdelete(joint); } else { - ERR_FAIL_MSG("Invalid ID."); + ERR_FAIL_MSG("Invalid RID."); } }; diff --git a/servers/physics/physics_server_sw.h b/servers/physics/physics_server_sw.h index 3a8fede2ab..770235dc6c 100644 --- a/servers/physics/physics_server_sw.h +++ b/servers/physics/physics_server_sw.h @@ -355,7 +355,7 @@ public: /* MISC */ - virtual void free(RID p_rid); + virtual void free(RID &p_rid); virtual void set_active(bool p_active); virtual void init(); diff --git a/servers/physics_2d_server.cpp b/servers/physics_2d_server.cpp index f34cb4c4ca..844948fa07 100644 --- a/servers/physics_2d_server.cpp +++ b/servers/physics_2d_server.cpp @@ -642,7 +642,7 @@ void Physics2DServer::_bind_methods() { ClassDB::bind_method(D_METHOD("joint_get_type", "joint"), &Physics2DServer::joint_get_type); - ClassDB::bind_method(D_METHOD("free_rid", "rid"), &Physics2DServer::free); + ClassDB::bind_method(D_METHOD("free_rid", "rid"), &Physics2DServer::free_rid); ClassDB::bind_method(D_METHOD("set_active", "active"), &Physics2DServer::set_active); diff --git a/servers/physics_2d_server.h b/servers/physics_2d_server.h index bd34a3a02f..a94c702325 100644 --- a/servers/physics_2d_server.h +++ b/servers/physics_2d_server.h @@ -548,6 +548,7 @@ public: /* MISC */ virtual void free(RID p_rid) = 0; + virtual void free_rid(RID p_rid) { free(p_rid); } // gdscript binding virtual void set_active(bool p_active) = 0; virtual void init() = 0; diff --git a/servers/physics_server.cpp b/servers/physics_server.cpp index 224773cea4..9cbc173777 100644 --- a/servers/physics_server.cpp +++ b/servers/physics_server.cpp @@ -720,7 +720,7 @@ void PhysicsServer::_bind_methods() { ClassDB::bind_method(D_METHOD("generic_6dof_joint_set_flag", "joint", "axis", "flag", "enable"), &PhysicsServer::generic_6dof_joint_set_flag); ClassDB::bind_method(D_METHOD("generic_6dof_joint_get_flag", "joint", "axis", "flag"), &PhysicsServer::generic_6dof_joint_get_flag); - ClassDB::bind_method(D_METHOD("free_rid", "rid"), &PhysicsServer::free); + ClassDB::bind_method(D_METHOD("free_rid", "rid"), &PhysicsServer::free_rid); ClassDB::bind_method(D_METHOD("set_active", "active"), &PhysicsServer::set_active); diff --git a/servers/physics_server.h b/servers/physics_server.h index 8afb4fd896..e51057cd4d 100644 --- a/servers/physics_server.h +++ b/servers/physics_server.h @@ -721,7 +721,8 @@ public: /* MISC */ - virtual void free(RID p_rid) = 0; + virtual void free(RID &p_rid) = 0; + virtual void free_rid(RID p_rid) { free(p_rid); } // gdscript binding virtual void set_active(bool p_active) = 0; virtual void init() = 0; diff --git a/servers/visual/rasterizer.h b/servers/visual/rasterizer.h index 452662c275..1224a9a352 100644 --- a/servers/visual/rasterizer.h +++ b/servers/visual/rasterizer.h @@ -169,7 +169,7 @@ public: virtual void set_scene_pass(uint64_t p_pass) = 0; virtual void set_debug_draw_mode(VS::ViewportDebugDraw p_debug_draw) = 0; - virtual bool free(RID p_rid) = 0; + virtual bool free(RID &p_rid) = 0; virtual ~RasterizerScene() {} }; @@ -595,7 +595,7 @@ public: virtual void canvas_light_occluder_set_polylines(RID p_occluder, const PoolVector &p_lines) = 0; virtual VS::InstanceType get_base_type(RID p_rid) const = 0; - virtual bool free(RID p_rid) = 0; + virtual bool free(RID &p_rid) = 0; virtual bool has_os_feature(const String &p_feature) const = 0; diff --git a/servers/visual/visual_server_canvas.cpp b/servers/visual/visual_server_canvas.cpp index 6a32577327..b55e8c790b 100644 --- a/servers/visual/visual_server_canvas.cpp +++ b/servers/visual/visual_server_canvas.cpp @@ -1242,7 +1242,7 @@ void VisualServerCanvas::canvas_occluder_polygon_set_cull_mode(RID p_occluder_po } } -bool VisualServerCanvas::free(RID p_rid) { +bool VisualServerCanvas::free(RID &p_rid) { if (canvas_owner.owns(p_rid)) { Canvas *canvas = canvas_owner.get(p_rid); ERR_FAIL_COND_V(!canvas, false); diff --git a/servers/visual/visual_server_canvas.h b/servers/visual/visual_server_canvas.h index 43b1e6c21c..8c27d396e0 100644 --- a/servers/visual/visual_server_canvas.h +++ b/servers/visual/visual_server_canvas.h @@ -253,7 +253,7 @@ public: void canvas_occluder_polygon_set_cull_mode(RID p_occluder_polygon, VS::CanvasOccluderPolygonCullMode p_mode); - bool free(RID p_rid); + bool free(RID &p_rid); VisualServerCanvas(); ~VisualServerCanvas(); }; diff --git a/servers/visual/visual_server_raster.cpp b/servers/visual/visual_server_raster.cpp index 25a4e655d4..5fdf782496 100644 --- a/servers/visual/visual_server_raster.cpp +++ b/servers/visual/visual_server_raster.cpp @@ -64,7 +64,11 @@ void VisualServerRaster::_draw_margins() { /* FREE */ -void VisualServerRaster::free(RID p_rid) { +void VisualServerRaster::free(RID &p_rid) { + if (! p_rid.is_valid() || p_rid.get_id() == 0) { + return; + } + if (VSG::storage->free(p_rid)) { return; } @@ -195,6 +199,7 @@ void VisualServerRaster::call_set_use_vsync(bool p_enable) { bool VisualServerRaster::is_low_end() const { return VSG::rasterizer->is_low_end(); } + VisualServerRaster::VisualServerRaster() { VSG::canvas = memnew(VisualServerCanvas); VSG::viewport = memnew(VisualServerViewport); diff --git a/servers/visual/visual_server_raster.h b/servers/visual/visual_server_raster.h index 9e7c754206..3d8c54e4a2 100644 --- a/servers/visual/visual_server_raster.h +++ b/servers/visual/visual_server_raster.h @@ -724,7 +724,7 @@ public: /* FREE */ - virtual void free(RID p_rid); ///< free RIDs associated with the visual server + virtual void free(RID &p_rid); ///< free RIDs associated with the visual server /* EVENT QUEUING */ diff --git a/servers/visual/visual_server_scene.cpp b/servers/visual/visual_server_scene.cpp index f80814b89c..cc152b9234 100644 --- a/servers/visual/visual_server_scene.cpp +++ b/servers/visual/visual_server_scene.cpp @@ -4028,7 +4028,7 @@ void VisualServerScene::update_dirty_instances() { } } -bool VisualServerScene::free(RID p_rid) { +bool VisualServerScene::free(RID &p_rid) { if (camera_owner.owns(p_rid)) { Camera *camera = camera_owner.get(p_rid); @@ -4047,8 +4047,6 @@ bool VisualServerScene::free(RID p_rid) { memdelete(scenario); } else if (instance_owner.owns(p_rid)) { - // delete the instance - update_dirty_instances(); Instance *instance = instance_owner.get(p_rid); @@ -4063,26 +4061,32 @@ bool VisualServerScene::free(RID p_rid) { instance_owner.free(p_rid); memdelete(instance); + } else if (room_owner.owns(p_rid)) { Room *room = room_owner.get(p_rid); room_owner.free(p_rid); memdelete(room); + } else if (portal_owner.owns(p_rid)) { Portal *portal = portal_owner.get(p_rid); portal_owner.free(p_rid); memdelete(portal); + } else if (ghost_owner.owns(p_rid)) { Ghost *ghost = ghost_owner.get(p_rid); ghost_owner.free(p_rid); memdelete(ghost); + } else if (roomgroup_owner.owns(p_rid)) { RoomGroup *roomgroup = roomgroup_owner.get(p_rid); roomgroup_owner.free(p_rid); memdelete(roomgroup); + } else if (occluder_owner.owns(p_rid)) { Occluder *ro = occluder_owner.get(p_rid); occluder_owner.free(p_rid); memdelete(ro); + } else { return false; } diff --git a/servers/visual/visual_server_scene.h b/servers/visual/visual_server_scene.h index ca5b2285dc..0e9534d237 100644 --- a/servers/visual/visual_server_scene.h +++ b/servers/visual/visual_server_scene.h @@ -755,7 +755,7 @@ public: void render_probes(); - bool free(RID p_rid); + bool free(RID &p_rid); private: bool _use_bvh; diff --git a/servers/visual/visual_server_viewport.cpp b/servers/visual/visual_server_viewport.cpp index 9ab26cf544..e8e9d4cc88 100644 --- a/servers/visual/visual_server_viewport.cpp +++ b/servers/visual/visual_server_viewport.cpp @@ -710,7 +710,7 @@ void VisualServerViewport::viewport_set_debug_draw(RID p_viewport, VS::ViewportD viewport->debug_draw = p_draw; } -bool VisualServerViewport::free(RID p_rid) { +bool VisualServerViewport::free(RID &p_rid) { if (viewport_owner.owns(p_rid)) { Viewport *viewport = viewport_owner.getornull(p_rid); diff --git a/servers/visual/visual_server_viewport.h b/servers/visual/visual_server_viewport.h index 201d1c70e6..9395ace988 100644 --- a/servers/visual/visual_server_viewport.h +++ b/servers/visual/visual_server_viewport.h @@ -198,7 +198,7 @@ public: void set_default_clear_color(const Color &p_color); void draw_viewports(); - bool free(RID p_rid); + bool free(RID &p_rid); VisualServerViewport(); virtual ~VisualServerViewport() {} diff --git a/servers/visual/visual_server_wrap_mt.h b/servers/visual/visual_server_wrap_mt.h index fe252639bb..fd744dd071 100644 --- a/servers/visual/visual_server_wrap_mt.h +++ b/servers/visual/visual_server_wrap_mt.h @@ -643,7 +643,7 @@ public: /* FREE */ - FUNC1(free, RID) + FUNC1(free, RID &) /* EVENT QUEUING */ diff --git a/servers/visual_server.cpp b/servers/visual_server.cpp index 45e9bd55c6..affd8f93e6 100644 --- a/servers/visual_server.cpp +++ b/servers/visual_server.cpp @@ -2226,7 +2226,7 @@ void VisualServer::_bind_methods() { ClassDB::bind_method(D_METHOD("black_bars_set_margins", "left", "top", "right", "bottom"), &VisualServer::black_bars_set_margins); ClassDB::bind_method(D_METHOD("black_bars_set_images", "left", "top", "right", "bottom"), &VisualServer::black_bars_set_images); - ClassDB::bind_method(D_METHOD("free_rid", "rid"), &VisualServer::free); // shouldn't conflict with Object::free() + ClassDB::bind_method(D_METHOD("free_rid", "rid"), &VisualServer::free_rid); // shouldn't conflict with Object::free() ClassDB::bind_method(D_METHOD("request_frame_drawn_callback", "where", "method", "userdata"), &VisualServer::request_frame_drawn_callback); ClassDB::bind_method(D_METHOD("has_changed"), &VisualServer::has_changed); diff --git a/servers/visual_server.h b/servers/visual_server.h index b6da7a9136..38cebfd629 100644 --- a/servers/visual_server.h +++ b/servers/visual_server.h @@ -1092,8 +1092,8 @@ public: /* FREE */ - virtual void free(RID p_rid) = 0; ///< free RIDs associated with the visual server - + virtual void free(RID &p_rid) = 0; ///< free RIDs associated with the visual server + virtual void free_rid(RID p_rid) { free(p_rid); } // for gdscript binding virtual void request_frame_drawn_callback(Object *p_where, const StringName &p_method, const Variant &p_userdata) = 0; /* EVENT QUEUING */ From 7778b7459acb8a6e686fba30b0d6b2309491c513 Mon Sep 17 00:00:00 2001 From: Cory Petkovsek <632766+tinmanjuggernaut@users.noreply.github.com> Date: Wed, 10 Nov 2021 22:06:59 +0800 Subject: [PATCH 2/2] Remove try/catch in RID. Updated docs --- core/rid.h | 13 ++----------- doc/classes/PhysicsServer.xml | 4 ++-- doc/classes/VisualServer.xml | 4 ++-- 3 files changed, 6 insertions(+), 15 deletions(-) diff --git a/core/rid.h b/core/rid.h index 5a58253ea5..2b793b0b19 100644 --- a/core/rid.h +++ b/core/rid.h @@ -78,16 +78,7 @@ public: } _FORCE_INLINE_ bool is_valid() const { return _data != nullptr; } - _FORCE_INLINE_ uint32_t get_id() const { - if (_data) { - try { - return _data->get_id(); - } catch (...) { - ERR_FAIL_V_MSG(0, "RID points to memory that has already been freed."); - } - } - return 0; // An invalid or uninitialized state - } + _FORCE_INLINE_ uint32_t get_id() const { return _data ? _data->get_id() : 0; } _FORCE_INLINE_ RID() { _data = nullptr; @@ -156,7 +147,7 @@ public: } void get_owned_list(List *p_owned) { - // Used to be a debug only function disabled during release mode. Now deprecated. + // Used to be a debug only function disabled during release mode. Now deprecated. } }; diff --git a/doc/classes/PhysicsServer.xml b/doc/classes/PhysicsServer.xml index 034a45effd..33c380fd34 100644 --- a/doc/classes/PhysicsServer.xml +++ b/doc/classes/PhysicsServer.xml @@ -634,8 +634,8 @@ [code] var r:RID = PhysicsServer.space_create() PhysicsServer.free_rid(r) - print("ID: ", r.get_id()) # Not safe to access or free an invalid RID - r = RID() # Reset RID, safe to use or free again + print("ID: ", r.get_id()) # It is not safe to access or free an invalid RID + r = RID() # Reset the RID so it's safe to use or free again print("ID: ", r.get_id()) Output: diff --git a/doc/classes/VisualServer.xml b/doc/classes/VisualServer.xml index 25f61a4572..a35b4749ee 100644 --- a/doc/classes/VisualServer.xml +++ b/doc/classes/VisualServer.xml @@ -966,8 +966,8 @@ [code] var r:RID = VisualServer.get_test_cube() VisualServer.free_rid(r) - print("ID: ", r.get_id()) # Not safe to access or free an invalid RID - r = RID() # Reset RID, safe to use or free again + print("ID: ", r.get_id()) # It is not safe to access or free an invalid RID + r = RID() # Reset the RID so it's safe to use or free again print("ID: ", r.get_id()) Output: