#5 Make automatic_rotate relative, allow setting rotation

Open
pgimeno wants to merge 1 commits from pgimeno/pg-auto-rotate-relative into pgimeno/master

automatic_rotate does not make sense if it is absolute. Make it relative.

To avoid bouncing, set_rotation did not update the client when automatic_rotate was set. That's no longer necessary because the new spinning method applies the rotation on top of the current one, and the updates are necessary for set_rotation to actually transform the object.

The problem fixed here is the result of an oversight when set_rotation was added. Objects couldn't be oriented in any other way before it was implemented, therefore this wasn't an issue.

This allows for single-object rotation as discussed in https://github.com/minetest/minetest/issues/8456, when it applies to objects that are not attached.

Full axis specification, as discussed there, while interesting, is not strictly necessary. As long as the model is constructed in such a way that it's designed to spin over its local Y axis (for example, making a propeller such that it "lies" on the XZ plane and with the rotation axis being the Y axis), it's possible to use set_rotation to reorient the object so that the rotation axis points in the desired direction.

Therefore, full axis specification would only be helpful in case the model does not meet that requirement. Assuming the creator has full control over the model, that's not necessary.

What is not covered by this patch is rotation of attachments. This means that e.g. a propeller attached to a plane is not possible without adding a new property. Current plans are to submit a separate PR for that.

`automatic_rotate` does not make sense if it is absolute. Make it relative. To avoid bouncing, `set_rotation` did not update the client when `automatic_rotate` was set. That's no longer necessary because the new spinning method applies the rotation on top of the current one, and the updates are necessary for `set_rotation` to actually transform the object. The problem fixed here is the result of an oversight when `set_rotation` was added. Objects couldn't be oriented in any other way before it was implemented, therefore this wasn't an issue. This allows for single-object rotation as discussed in https://github.com/minetest/minetest/issues/8456, when it applies to objects that are not attached. Full axis specification, as discussed there, while interesting, is not strictly necessary. As long as the model is constructed in such a way that it's designed to spin over its local Y axis (for example, making a propeller such that it "lies" on the XZ plane and with the rotation axis being the Y axis), it's possible to use `set_rotation` to reorient the object so that the rotation axis points in the desired direction. Therefore, full axis specification would only be helpful in case the model does not meet that requirement. Assuming the creator has full control over the model, that's not necessary. What is not covered by this patch is rotation of attachments. This means that e.g. a propeller attached to a plane is not possible without adding a new property. Current plans are to submit a separate PR for that.
Pedro Gimeno commented 5 years ago
Owner

I've experimented with automatic_rot_attached and got a working patch. However, when attached to the player's arm, for some reason the object switches arm during a frame several times for a while. I have no idea where to start to debug this problem. With some luck it won't happen with other models.

Save for that, the patch is almost trivial, but since it depends on this one, it needs to wait until this is merged. Here it is in advance:

commit 1eafffaf84860d9f17b688aac40d0a9a0d5e51fa
Author: Pedro Gimeno <pgimeno@users.noreply.notabug.org>
Date:   Tue Apr 9 23:36:28 2019 +0200

    Add automatic_rot_attached property for attached objects

diff --git a/src/client/content_cao.cpp b/src/client/content_cao.cpp
index ec9dc7a2..3a508f3c 100644
--- a/src/client/content_cao.cpp
+++ b/src/client/content_cao.cpp
@@ -993,7 +993,7 @@ void GenericCAO::step(float dtime, ClientEnvironment *env)
 	// This is the child node's rotation. It is only used for automatic_rotate.
 	v3f local_rot = node->getRotation();
 	local_rot.Y = modulo360f(local_rot.Y - dtime * core::RADTODEG *
-			(getParent() ? 0.f : m_prop.automatic_rotate));
+			(getParent() ? m_prop.automatic_rot_attached : m_prop.automatic_rotate));
 	node->setRotation(local_rot);
 
 	if (!getParent() && m_prop.automatic_face_movement_dir &&
diff --git a/src/object_properties.cpp b/src/object_properties.cpp
index a037c5f6..ab0f352c 100644
--- a/src/object_properties.cpp
+++ b/src/object_properties.cpp
@@ -69,6 +69,7 @@ std::string ObjectProperties::dump()
 	os << ", eye_height=" << eye_height;
 	os << ", zoom_fov=" << zoom_fov;
 	os << ", use_texture_alpha=" << use_texture_alpha;
+	os << ", automatic_rot_attached=" << automatic_rot_attached;
 	return os.str();
 }
 
@@ -115,6 +116,7 @@ void ObjectProperties::serialize(std::ostream &os) const
 	writeF32(os, eye_height);
 	writeF32(os, zoom_fov);
 	writeU8(os, use_texture_alpha);
+	writeF32(os, automatic_rot_attached);
 
 	// Add stuff only at the bottom.
 	// Never remove anything, because we don't want new versions of this
@@ -167,4 +169,5 @@ void ObjectProperties::deSerialize(std::istream &is)
 	eye_height = readF32(is);
 	zoom_fov = readF32(is);
 	use_texture_alpha = readU8(is);
+	automatic_rot_attached = readF32(is);
 }
diff --git a/src/object_properties.h b/src/object_properties.h
index 199182d7..d0a5613b 100644
--- a/src/object_properties.h
+++ b/src/object_properties.h
@@ -47,6 +47,7 @@ struct ObjectProperties
 	bool makes_footstep_sound = false;
 	f32 stepheight = 0.0f;
 	float automatic_rotate = 0.0f;
+	float automatic_rot_attached = 0.0f;
 	bool automatic_face_movement_dir = false;
 	f32 automatic_face_movement_dir_offset = 0.0f;
 	bool backface_culling = true;
diff --git a/src/script/common/c_content.cpp b/src/script/common/c_content.cpp
index 361db226..71a30adf 100644
--- a/src/script/common/c_content.cpp
+++ b/src/script/common/c_content.cpp
@@ -278,6 +278,7 @@ void read_object_properties(lua_State *L, int index,
 	getfloatfield(L, -1, "eye_height", prop->eye_height);
 
 	getfloatfield(L, -1, "automatic_rotate", prop->automatic_rotate);
+	getfloatfield(L, -1, "automatic_rot_attached", prop->automatic_rot_attached);
 	lua_getfield(L, -1, "automatic_face_movement_dir");
 	if (lua_isnumber(L, -1)) {
 		prop->automatic_face_movement_dir = true;
@@ -374,6 +375,8 @@ void push_object_properties(lua_State *L, ObjectProperties *prop)
 	lua_setfield(L, -2, "eye_height");
 	lua_pushnumber(L, prop->automatic_rotate);
 	lua_setfield(L, -2, "automatic_rotate");
+	lua_pushnumber(L, prop->automatic_rot_attached);
+	lua_setfield(L, -2, "automatic_rot_attached");
 	if (prop->automatic_face_movement_dir)
 		lua_pushnumber(L, prop->automatic_face_movement_dir_offset);
 	else
I've experimented with `automatic_rot_attached` and got a working patch. However, when attached to the player's arm, for some reason the object switches arm during a frame several times for a while. I have no idea where to start to debug this problem. With some luck it won't happen with other models. Save for that, the patch is almost trivial, but since it depends on this one, it needs to wait until this is merged. Here it is in advance: ``` commit 1eafffaf84860d9f17b688aac40d0a9a0d5e51fa Author: Pedro Gimeno <pgimeno@users.noreply.notabug.org> Date: Tue Apr 9 23:36:28 2019 +0200 Add automatic_rot_attached property for attached objects diff --git a/src/client/content_cao.cpp b/src/client/content_cao.cpp index ec9dc7a2..3a508f3c 100644 --- a/src/client/content_cao.cpp +++ b/src/client/content_cao.cpp @@ -993,7 +993,7 @@ void GenericCAO::step(float dtime, ClientEnvironment *env) // This is the child node's rotation. It is only used for automatic_rotate. v3f local_rot = node->getRotation(); local_rot.Y = modulo360f(local_rot.Y - dtime * core::RADTODEG * - (getParent() ? 0.f : m_prop.automatic_rotate)); + (getParent() ? m_prop.automatic_rot_attached : m_prop.automatic_rotate)); node->setRotation(local_rot); if (!getParent() && m_prop.automatic_face_movement_dir && diff --git a/src/object_properties.cpp b/src/object_properties.cpp index a037c5f6..ab0f352c 100644 --- a/src/object_properties.cpp +++ b/src/object_properties.cpp @@ -69,6 +69,7 @@ std::string ObjectProperties::dump() os << ", eye_height=" << eye_height; os << ", zoom_fov=" << zoom_fov; os << ", use_texture_alpha=" << use_texture_alpha; + os << ", automatic_rot_attached=" << automatic_rot_attached; return os.str(); } @@ -115,6 +116,7 @@ void ObjectProperties::serialize(std::ostream &os) const writeF32(os, eye_height); writeF32(os, zoom_fov); writeU8(os, use_texture_alpha); + writeF32(os, automatic_rot_attached); // Add stuff only at the bottom. // Never remove anything, because we don't want new versions of this @@ -167,4 +169,5 @@ void ObjectProperties::deSerialize(std::istream &is) eye_height = readF32(is); zoom_fov = readF32(is); use_texture_alpha = readU8(is); + automatic_rot_attached = readF32(is); } diff --git a/src/object_properties.h b/src/object_properties.h index 199182d7..d0a5613b 100644 --- a/src/object_properties.h +++ b/src/object_properties.h @@ -47,6 +47,7 @@ struct ObjectProperties bool makes_footstep_sound = false; f32 stepheight = 0.0f; float automatic_rotate = 0.0f; + float automatic_rot_attached = 0.0f; bool automatic_face_movement_dir = false; f32 automatic_face_movement_dir_offset = 0.0f; bool backface_culling = true; diff --git a/src/script/common/c_content.cpp b/src/script/common/c_content.cpp index 361db226..71a30adf 100644 --- a/src/script/common/c_content.cpp +++ b/src/script/common/c_content.cpp @@ -278,6 +278,7 @@ void read_object_properties(lua_State *L, int index, getfloatfield(L, -1, "eye_height", prop->eye_height); getfloatfield(L, -1, "automatic_rotate", prop->automatic_rotate); + getfloatfield(L, -1, "automatic_rot_attached", prop->automatic_rot_attached); lua_getfield(L, -1, "automatic_face_movement_dir"); if (lua_isnumber(L, -1)) { prop->automatic_face_movement_dir = true; @@ -374,6 +375,8 @@ void push_object_properties(lua_State *L, ObjectProperties *prop) lua_setfield(L, -2, "eye_height"); lua_pushnumber(L, prop->automatic_rotate); lua_setfield(L, -2, "automatic_rotate"); + lua_pushnumber(L, prop->automatic_rot_attached); + lua_setfield(L, -2, "automatic_rot_attached"); if (prop->automatic_face_movement_dir) lua_pushnumber(L, prop->automatic_face_movement_dir_offset); else ```
Pedro Gimeno commented 5 years ago
Owner

Note: This PR builds on the existing property automatic_rotate which already rotates on Y. It's compatible with the old behaviour. That's the reason for having to build the models with a vertical rotation axis in mind.

Rotating properly around an arbitrary axis without accumulated error, while possible, would make the patch a lot more complex, at least without inserting another extra IDummyTransformationSceneNode. It would be easy to rotate over any of the three main axes, though, not just Y, in a way similar to what ClobberXD suggested. They would rotate in Irrlicht XYZ extrinsic Euler order. Rotating over more than one axis, even if not at the same time, would lead to issues.

Note: This PR builds on the *existing* property `automatic_rotate` which already rotates on Y. It's compatible with the old behaviour. That's the reason for having to build the models with a vertical rotation axis in mind. Rotating properly around an arbitrary axis without accumulated error, while possible, would make the patch a lot more complex, at least without inserting another extra `IDummyTransformationSceneNode`. It would be easy to rotate over any of the three main axes, though, not just Y, in a way similar to what ClobberXD suggested. They would rotate in Irrlicht XYZ extrinsic Euler order. Rotating over more than one axis, even if not at the same time, would lead to issues.
This pull request can't be merged automatically because there are conflicts.
Please merge manually in order to resolve the conflicts.
Sign in to join this conversation.
No Milestone
No assignee
1 Participants
Loading...
Cancel
Save
There is no content yet.