[MTE 5.5] Update deprecated vector codestyle. #1860

New Issue

Ghost · 2021-09-10T05:51:13+02:00

Ghost commented

2021-09-10 05:51:13 +02:00

In the next MTE 5.5 release, as stated in MTE/PR#11039:

Internally, (vector) is implemented as a table with the 3 fields
x, y and z. Example: {x = 0, y = 1, z = 0}.

However, one should never create a vector manually as above, such misbehavior is deprecated. The vector helpers set a metatable for the created vectors which allows indexing with numbers, calling functions directly on vectors and using operators (like +). Furthermore, the internal implementation might change in the future.

Old code might still use vectors without metatables, be aware of this!

We will need to update all manual-style vectors to fit with the new-style vectors.

In the next MTE 5.5 release, as stated in [MTE/PR#11039](https://github.com/minetest/minetest/pull/11039)\: > Internally, (vector) is implemented as a table with the 3 fields `x`, `y` and `z`. Example: `{x = 0, y = 1, z = 0}`. > However, one should *never* create a vector manually as above, such misbehavior is deprecated. The vector helpers set a metatable for the created vectors which allows indexing with numbers, calling functions directly on vectors and using operators (like `+`). Furthermore, the internal implementation might change in the future. > Old code might still use vectors without metatables, be aware of this! We will need to update all manual-style vectors to fit with the new-style vectors.

Ghost added the

delayed for engine release

code quality

#P3: elevated

labels 2021-09-10 05:51:13 +02:00

AFCMS commented

2021-09-10 08:58:29 +02:00

vector.new is availlable in mt < 5.5, so we can already support 5.5, right?

`vector.new` is availlable in mt < 5.5, so we can already support 5.5, right?

Ghost commented

2021-09-10 17:19:01 +02:00

we also have new metatable operations in 5.5. so instead of vector.add(p, d), it could be written simpler as p + d (the new-style could be slower though bc metatables. someone would need to profile that, if new-style is adopted).

anyways, you're right, we can support 5.5 now. but i'd rather wait until 5.5 is released in case there's more changes.

we also have new metatable operations in 5.5. so instead of `vector.add(p, d)`, it could be written simpler as `p + d` (the new-style could be slower though bc metatables. someone would need to profile that, if new-style is adopted). anyways, you're right, we can support 5.5 now. but i'd rather wait until 5.5 is released in case there's more changes.

👍 1

kneekoo commented

2021-09-22 01:01:19 +02:00

anyways, you're right, we can support 5.5 now. but i'd rather wait until 5.5 is released in case there's more changes.

That's true if they make any other changes to the vector codestyle but it was reviewed, tweaked and tested, and then merged. It looks like whatever changes follow, they're unlikely to break compatibility.

> anyways, you're right, we can support 5.5 now. but i'd rather wait until 5.5 is released in case there's more changes. That's true *if* they make any other changes to the vector codestyle but it was reviewed, tweaked and tested, and then merged. It looks like whatever changes follow, they're unlikely to break compatibility.

Ghost removed the

delayed for engine release

label 2021-09-27 19:02:53 +02:00

Ghost commented

2021-09-27 19:19:07 +02:00

fleckenstein: mcl 0.72 will support only mt 5.5 btw

with this, we could just start doing it now (and because MTE 5.5 is close to release).

> fleckenstein: mcl 0.72 will support only mt 5.5 btw with this, we could just start doing it now (and because MTE 5.5 is close to release).

AFCMS commented

2021-10-01 11:36:24 +02:00

hum... maybe not on master, but on a separated branch yes (I still often use 5.4.1)

AFCMS commented

2021-10-01 11:36:42 +02:00

@EliasFleckenstein03 thoughts?

AFCMS referenced this issue

2021-10-18 09:13:32 +02:00

[MTE 5.5] Code Quality TODO #1877

LizzyFleckenstein03 commented

2021-10-24 17:26:03 +02:00

We should not work on this right now, since vector is an essential thing used in many different places. Let's wait until a good portion of the 0.72 proposed changes are done and merged and then work on it to not get loads of merge conflicts that have to be resolved manually.

👍 1

kay27 commented

2022-01-20 13:06:41 +01:00

We use absolute world position, always - be it vectors, tables, areas, direct coordinates specifications, whatever.

We don't use relative vectors for now.

Let's wait final 5.5 and then check if we will start using them.

The issue is invalid for now.

No need to use vectors instead of tables right now. There more likely can be problems with new vectors than with tables we use, and a quick fix could be backmerging old vector library from 5.4 into mcl2, but it's about future prediction which isn't a goal

We use absolute world position, always - be it vectors, tables, areas, direct coordinates specifications, whatever. We don't use relative vectors for now. Let's wait final 5.5 and then check if we will start using them. The issue is invalid for now. No need to use vectors instead of tables right now. There more likely can be problems with new vectors than with tables we use, and a quick fix could be backmerging old vector library from 5.4 into mcl2, but it's about future prediction which isn't a goal

👎 1

kay27 closed this issue

2022-01-20 13:06:45 +01:00

kay27 added the

invalid / won't fix

label 2022-01-20 13:07:45 +01:00

AFCMS commented

2022-01-21 08:58:28 +01:00

Not using new style vectors will be deprecated by 5.5

From our CONTRIBUTING.md, we must use modern API

positions are vectors (see lua_api.txt)

The issue is fully valid and must be reopened

Not using new style vectors will be deprecated by 5.5 From our [CONTRIBUTING.md](https://git.minetest.land/MineClone2/MineClone2/src/branch/master/CONTRIBUTING.md#code-guidelines), we must **use modern API** positions are vectors (see lua_api.txt) The issue is fully valid and must be reopened

AFCMS reopened this issue

2022-01-21 10:37:26 +01:00

AFCMS removed the

invalid / won't fix

label 2022-01-21 10:37:41 +01:00

ancientmarinerdev removed the

#P3: elevated

label 2023-02-04 03:32:06 +01:00

ancientmarinerdev added the

#P3: elevated

label 2023-03-08 16:25:26 +01:00

ancientmarinerdev commented

2023-03-08 16:32:51 +01:00

My position has moved on this.

When working on the error handling for mobs, I realised that if you set an incorrect vector in the new style, it fails on creation as it uses an assert method. This is really quite significant. When you set a variable in 20 places, if it's a table, it will succeed and fail when it tries to read. Good luck trying to figure out where it got incorrectly set (this happened in an unfixed crash (for self.acc). Using new style vectors will fail early and tell you exactly where you are incorrectly using it. This prevents data getting into a bad state, and us crossing our fingers and hoping it's good.

We shouldn't always dive to migrate because something is deprecated, as things can be depracated for years, but this is a superior option and makes sense why they have done this.

Moving across to new vectors will decrease risk where a vector setting operation goes awry for a particular case or due to incorrect state. There is a lot of value in this and I think we do need to start migrating across.

Obviously big PRs are a nightmare to review and a merge conflict waiting to happen, so small chunks for this will be useful, and probably not all at once so it isn't exhausting to review/test.

We need to consider moving forward with this. I appreciate AFCMS was definitely leading the charge on this at one point, and you were spot on with that! So credit where it is due. If I had discouraged you in the past in regards to this, I apologise.

My position has moved on this. When working on the error handling for mobs, I realised that if you set an incorrect vector in the new style, it fails on creation as it uses an assert method. This is really quite significant. When you set a variable in 20 places, if it's a table, it will succeed and fail when it tries to read. Good luck trying to figure out where it got incorrectly set (this happened in an unfixed crash (for self.acc). Using new style vectors will fail early and tell you exactly where you are incorrectly using it. This prevents data getting into a bad state, and us crossing our fingers and hoping it's good. We shouldn't always dive to migrate because something is deprecated, as things can be depracated for years, but this is a superior option and makes sense why they have done this. Moving across to new vectors will decrease risk where a vector setting operation goes awry for a particular case or due to incorrect state. There is a lot of value in this and I think we do need to start migrating across. Obviously big PRs are a nightmare to review and a merge conflict waiting to happen, so small chunks for this will be useful, and probably not all at once so it isn't exhausting to review/test. We need to consider moving forward with this. I appreciate AFCMS was definitely leading the charge on this at one point, and you were spot on with that! So credit where it is due. If I had discouraged you in the past in regards to this, I apologise.

👍 1 ❤️ 1

ancientmarinerdev added the

looking for contributor

label 2023-04-05 14:08:15 +02:00

ancientmarinerdev added this to the 0.85.0 - Fire and Stone milestone 2023-04-27 17:26:41 +02:00

ancientmarinerdev modified the milestone from 0.85.0 - Fire and Stone to 2 - Next

2023-05-14 23:03:30 +02:00

Sign in to join this conversation.