#5 Make automatic_rotate relative, allow setting rotation

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.

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

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.