VoxeLibre/VoxeLibre
VoxeLibre
/
VoxeLibre
58
133
Fork 174
Code Issues 975 Pull Requests 102 Projects 4 Releases Wiki Activity

Refactor compass code. #2197

Merged
cora merged 10 commits from get-compass-image-fix into master 2022-05-12 23:37:18 +02:00
Conversation 44 Commits 10 Files Changed 11 +281 -151
kabou commented 2022-05-10 23:13:45 +02:00
Contributor
Copy Link

This PR updates the lodestone code.

There are few user visible changes, all bugfixes:

  • Lodestone compasses should have a stack_max = 1.
  • Lodestone compasses inappropriately reuse compass descriptions.
  • Lodestone compasses and compasses had no usage description.

The main change visible to other mineclone code is the addition of a new API function mcl_compass.get_compass_itemname(pos, dir, itemstack). The currently released version of mineclone still exports the function mcl_compass.get_compass_image(pos, dir) but this was modified by the initial lodestone addition PR to now become mcl_compass.get_compass_image(pos, dir, itemstack). Although the handling of a possible missing itemname in callers that have not been updated is gentle and at most causes a warning in the log and the compass image to be random, IMHO it is better to leave the current API unchanged (with limited functionality for lodestones) and provide a newer, shinier API altogether.

Since the lodestone addition has not yet been published, I would like to see the API be updated as described above before the next release is made and the lodestone changes to the compass API are published in either form.

[edit]Update:
The old exported function has now been reverted back to the format it has in the last released mineclone2 and remains available for external users. The only current user of the compass API in mineclone2, mcl_itemframes, has been updated to the new API function.

A summary "mcl_compass/API.md" has been added.

To be considered is the current far node handling in mcl_compass. I outlined some alternatives in MineClone2/MineClone2#2138 (comment) and will pursue this alternative implementation. If my efforts are successful, they may be added to this PR, or be put in a new PR.

Changelog so far:

  • Split up get_compass_image() into smaller functions. This allows
    for better code sharing between old and new API and globalstep fn.
  • Add get_compass_itemname() function. It will be the new API of
    choice, `get_compass_image() will be deprecated soon.
  • Remove function declaration out of globalstep function.
  • Various other performance improvements.
  • Add local aliases for global functions
  • Lodestone compasses can only stack 1 item.
  • Document functions and variables.
  • Fix lodetone compass inaccurately reusing compass descriptions.
  • Add usage descriptions to node definitions
  • Refactor craftitem registration code.
  • Update translation templates.

Add new compass API.

  • Add API.md
  • Update mcl_itemframes to use the new API.
  • Revert old exported function back to original API.
This PR updates the lodestone code. There are few user visible changes, all bugfixes: * Lodestone compasses should have a `stack_max = 1`. * Lodestone compasses inappropriately reuse compass descriptions. * Lodestone compasses and compasses had no usage description. The main change visible to other mineclone code is the addition of a new API function `mcl_compass.get_compass_itemname(pos, dir, itemstack)`. The currently released version of mineclone still exports the function `mcl_compass.get_compass_image(pos, dir)` but this was modified by the initial lodestone addition PR to now become `mcl_compass.get_compass_image(pos, dir, itemstack)`. Although the handling of a possible missing `itemname` in callers that have not been updated is gentle and at most causes a warning in the log and the compass image to be random, IMHO it is better to leave the current API unchanged (with limited functionality for lodestones) and provide a newer, shinier API altogether. Since the lodestone addition has not yet been published, I would like to see the API be updated as described above before the next release is made and the lodestone changes to the compass API are published in either form. [edit]Update: The old exported function has now been reverted back to the format it has in the last released mineclone2 and remains available for external users. The only current user of the compass API in mineclone2, `mcl_itemframes`, has been updated to the new API function. A summary "mcl_compass/API.md" has been added. To be considered is the current far node handling in mcl_compass. I outlined some alternatives in https://git.minetest.land/MineClone2/MineClone2/pulls/2138#issuecomment-37180 and will pursue this alternative implementation. If my efforts are successful, they may be added to this PR, or be put in a new PR. Changelog so far: * Split up `get_compass_image()` into smaller functions. This allows for better code sharing between old and new API and globalstep fn. * Add `get_compass_itemname()` function. It will be the new API of choice, `get_compass_image() will be deprecated soon. * Remove function declaration out of globalstep function. * Various other performance improvements. * Add local aliases for global functions * Lodestone compasses can only stack 1 item. * Document functions and variables. * Fix lodetone compass inaccurately reusing compass descriptions. * Add usage descriptions to node definitions * Refactor craftitem registration code. * Update translation templates. Add new compass API. * Add API.md * Update mcl_itemframes to use the new API. * Revert old exported function back to original API.
👍 2
kabou added 1 commit 2022-05-10 23:13:56 +02:00
a8c231da34 Refactor compass code. 
* Split up `get_compass_image()` into smaller functions.  This allows
  for better code sharing between old and new API and globalstep fn.
* Add `get_compass_itemname()` function.  It will be the new API of
  choice, `get_compass_image() will be deprecated soon.
* Remove function declaration out of globalstep function.
* Various other performance improvements.
* Add local aliases for global functions
* Lodestone compasses can only stack 1 item.
* Document functions and variables.
* Fix lodetone compass inaccurately reusing compass descriptions.
* Add usage descriptions to node definitions
* Refactor craftitem registration code.
* Update translation templates.
kabou added 1 commit 2022-05-10 23:40:16 +02:00
8a4b8707fa Add new compass API. 
* Add API.md
* Update mcl_itemframes to use the new API.
* Revert old exported function back to original API.
kabou changed title from WIP: Refactor compass code. to Refactor compass code. 2022-05-10 23:45:22 +02:00
kabou requested review from MysticTempest 2022-05-10 23:46:04 +02:00
kabou requested review from AFCMS 2022-05-10 23:46:05 +02:00
kabou requested review from cora 2022-05-10 23:46:05 +02:00
kabou requested review from NO11 2022-05-10 23:46:06 +02:00
kabou requested review from erlehmann 2022-05-10 23:46:06 +02:00
kabou requested review from chmodsayshello 2022-05-10 23:46:07 +02:00
kabou added the
API
code quality
 labels 2022-05-10 23:46:47 +02:00
cora commented 2022-05-11 00:01:43 +02:00
Contributor
Copy Link

I thought your idea for the far node handling was great - after all for a lodestone to disappear it has to be dug. This would make emerging completely unnecessary.

I do think however it should be put in a new pr. I'll check this out and merge if all works out.

I think it's 0.75 time soon and this looks like it should be in.

We've done a lot of stuff since 0.74

I thought your idea for the far node handling was great - after all for a lodestone to disappear it has to be dug. This would make emerging completely unnecessary. I do think however it should be put in a new pr. I'll check this out and merge if all works out. I think it's 0.75 time soon and this looks like it should be in. We've done a lot of stuff since 0.74
👍 1
kabou commented 2022-05-11 00:09:10 +02:00
Author
Contributor
Copy Link

Apart from doing the ususal luacheck, I did playtest the changes a bit and it behaved as it should.

Apart from doing the ususal luacheck, I did playtest the changes a bit and it behaved as it should.
cora commented 2022-05-11 00:52:13 +02:00
Contributor
Copy Link

crash in 5.4 when rightclicking lode stone:

mods/ITEMS/mcl_compass/init.lua:104: attempt to call upvalue 'vec_from_str' (a nil value)

why not use minetest.string_to_pos / pos_to_string like it did before ?

crash in 5.4 when rightclicking lode stone: `mods/ITEMS/mcl_compass/init.lua:104: attempt to call upvalue 'vec_from_str' (a nil value)` why not use minetest.string_to_pos / pos_to_string like it did before ?
cora commented 2022-05-11 01:17:12 +02:00
Contributor
Copy Link

works in 5.5 though

works in 5.5 though
erlehmann commented 2022-05-11 01:29:19 +02:00
Member
Copy Link

the handling of a possible missing itemname […] causes […] the compass image to be random

If this is meant to be a joke, I am not laughing right now.

> the handling of a possible missing itemname […] causes […] the compass image to be random If this is meant to be a joke, I am not laughing right now.
erlehmann commented 2022-05-11 01:32:58 +02:00
Member
Copy Link

Why exactly not put the lodestone stuff in a separate mod?

Why exactly not put the lodestone stuff in a *separate* mod?
cora commented 2022-05-11 02:09:06 +02:00
Contributor
Copy Link

Well i suppose you're a bit late to this particular party

Well i suppose you're a bit late to this particular party
erlehmann commented 2022-05-11 02:28:32 +02:00
Member
Copy Link

Well, I guess that means “same procedure as every other time, some clown had zero interest in maintainability”.

Well, I guess that means “same procedure as every other time, some clown had zero interest in maintainability”.
cora commented 2022-05-11 03:16:51 +02:00
Contributor
Copy Link

Uh p sure it is in mcl_compasses in mcl5 so – either way is not gonna be great for compatibility.

I think you're making too big a deal out of these.

Uh p sure it is in mcl_compasses in mcl5 so – either way is not gonna be great for compatibility. I think you're making too big a deal out of these.
cora commented 2022-05-11 03:20:38 +02:00
Contributor
Copy Link

We can't always put everything in a new mod otherwise in a few years we'll have 5 variants of every mod updated in each year.

And please do not use msdos or utf8 or something as an example where this was done. There are a number of significant differences to those projects wouldn't you think. The 2 most notable being the amount of people working on it and people using it.

I get your point but you're being extremist again. Also calling people clowns isn't exactly nice so pls

We can't always put everything in a new mod otherwise in a few years we'll have 5 variants of every mod updated in each year. And please do not use msdos or utf8 or something as an example where this was done. There are a number of significant differences to those projects wouldn't you think. The 2 most notable being the amount of people working on it and people using it. I get your point but you're being extremist again. Also calling people clowns isn't exactly nice so pls
👍 1
erlehmann commented 2022-05-11 09:15:38 +02:00
Member
Copy Link

We can't always put everything in a new mod otherwise in a few years we'll have 5 variants of every mod updated in each year.

The opposite is true – as actually, putting stuff in existing mods and keeping the same name leads to diverging versions of mods. If you wouly simply call this mod another name (like mcl_compass2) there would be zero compatibility problems. It is so simple, a name change! Wuzzy, for example recently realized that default in repixture should be called rp_default, as it has diverged too much from the default mod in Minetest Game.

By keeping the same name, yet incompatibly changing the API or dependency situation, you are not getting rid of dependency or compatibility problems. Instead, you are pretending, it does not exist.

And please do not use msdos or utf8 or something as an example where this was done. There are a number of significant differences to those projects wouldn't you think. The 2 most notable being the amount of people working on it and people using it.

I do not understand what you are trying to say here. Is this some kind of joke?

Also calling people clowns isn't exactly nice so pls

Okay.

> We can't always put everything in a new mod otherwise in a few years we'll have 5 variants of every mod updated in each year. The opposite is true – as actually, putting stuff in existing mods *and keeping the same name* leads to diverging versions of mods. If you wouly simply call this mod another name (like `mcl_compass2`) there would be zero compatibility problems. It is so simple, a name change! Wuzzy, for example recently realized that `default` in repixture should be called `rp_default`, as it has diverged too much from the `default` mod in Minetest Game. By keeping the same name, yet incompatibly changing the API or dependency situation, you are *not* getting rid of dependency or compatibility problems. Instead, you are pretending, it does not exist. > And please do not use msdos or utf8 or something as an example where this was done. There are a number of significant differences to those projects wouldn't you think. The 2 most notable being the amount of people working on it and people using it. I do not understand what you are trying to say here. Is this some kind of joke? > Also calling people clowns isn't exactly nice so pls Okay.
cora commented 2022-05-11 11:03:19 +02:00
Contributor
Copy Link

Ok. I actually do not understand what you're trying to say comparing mcl to msdos or utf8.

What you are proposing is absolutely ridiculous for this situation.

EDIT and probably one of the reasons mcla never rarely anything done i might add.

Ok. I actually do not understand what you're trying to say comparing mcl to msdos or utf8. What you are proposing is absolutely ridiculous for this situation. EDIT and probably one of the reasons mcla never rarely anything done i might add.
erlehmann commented 2022-05-11 13:50:23 +02:00
Member
Copy Link

Ok. I actually do not understand what you're trying to say comparing mcl to msdos or utf8.

I mentioned both of them when @kabou accused me of wanting to hinder innovation, as:

The capabilities of MSDOS were extended, but not incompatibly so.

UTF-8 extended ASCII in a backwards-compatible way.

I.e. both have innovation and compatibility.

> Ok. I actually do not understand what you're trying to say comparing mcl to msdos or utf8. I mentioned both of them when @kabou accused me of wanting to hinder innovation, as: The capabilities of MSDOS were extended, but not incompatibly so. UTF-8 extended ASCII in a backwards-compatible way. I.e. both have innovation *and* compatibility.
cora commented 2022-05-11 14:38:04 +02:00
Contributor
Copy Link

Yes and both have/had hundreds of people working on them and thousands and more using them.

Yes and both have/had hundreds of people working on them and thousands and more using them.
👍 1
erlehmann commented 2022-05-11 14:55:02 +02:00
Member
Copy Link

Yes and both have/had hundreds of people working on them and thousands and more using them.

UTF-8 was basically invented by some clever people on a napkin … after multiple standards groups had come up with several worse ideas for this type of encoding:

https://www.cl.cam.ac.uk/~mgk25/ucs/utf-8-history.txt

Subject: UTF-8 history
From: "Rob 'Commander' Pike" <r (at) google.com>
Date: Wed, 30 Apr 2003 22:32:32 -0700 (Thu 06:32 BST)
To: mkuhn (at) acm.org, henry (at) spsystems.net
Cc: ken (at) entrisphere.com

Looking around at some UTF-8 background, I see the same incorrect
story being repeated over and over.  The incorrect version is:
	1. IBM designed UTF-8.
	2. Plan 9 implemented it.
That's not true.  UTF-8 was designed, in front of my eyes, on a
placemat in a New Jersey diner one night in September or so 1992.

What happened was this.  We had used the original UTF from ISO 10646
to make Plan 9 support 16-bit characters, but we hated it.  We were
close to shipping the system when, late one afternoon, I received a
call from some folks, I think at IBM - I remember them being in Austin
- who were in an X/Open committee meeting.  They wanted Ken and me to
vet their FSS/UTF design.  We understood why they were introducing a
new design, and Ken and I suddenly realized there was an opportunity
to use our experience to design a really good standard and get the
X/Open guys to push it out.  We suggested this and the deal was, if we
could do it fast, OK.  So we went to dinner, Ken figured out the
bit-packing, and when we came back to the lab after dinner we called
the X/Open guys and explained our scheme.  We mailed them an outline
of our spec, and they replied saying that it was better than theirs (I
don't believe I ever actually saw their proposal; I know I don't
remember it) and how fast could we implement it?  I think this was a
Wednesday night and we promised a complete running system by Monday,
which I think was when their big vote was.

So that night Ken wrote packing and unpacking code and I started
tearing into the C and graphics libraries.  The next day all the code
was done and we started converting the text files on the system
itself.  By Friday some time Plan 9 was running, and only running,
what would be called UTF-8.  We called X/Open and the rest, as they
say, is slightly rewritten history.

Why didn't we just use their FSS/UTF?  As I remember, it was because
in that first phone call I sang out a list of desiderata for any such
encoding, and FSS/UTF was lacking at least one - the ability to
synchronize a byte stream picked up mid-run, with less that one
character being consumed before synchronization.  Becuase that was
lacking, we felt free - and were given freedom - to roll our own.

I think the "IBM designed it, Plan 9 implemented it" story originates
in RFC2279.  At the time, we were so happy UTF-8 was catching on we
didn't say anything about the bungled history.  Neither of us is at
the Labs any more, but I bet there's an e-mail thread in the archive
there that would support our story and I might be able to get someone
to dig it out.

So, full kudos to the X/Open and IBM folks for making the opportunity
happen and for pushing it forward, but Ken designed it with me
cheering him on, whatever the history books say.

-rob
> Yes and both have/had hundreds of people working on them and thousands and more using them. UTF-8 was basically invented by some clever people on a napkin … after multiple standards groups had come up with several worse ideas for this type of encoding: https://www.cl.cam.ac.uk/~mgk25/ucs/utf-8-history.txt ``` Subject: UTF-8 history From: "Rob 'Commander' Pike" <r (at) google.com> Date: Wed, 30 Apr 2003 22:32:32 -0700 (Thu 06:32 BST) To: mkuhn (at) acm.org, henry (at) spsystems.net Cc: ken (at) entrisphere.com Looking around at some UTF-8 background, I see the same incorrect story being repeated over and over. The incorrect version is: 1. IBM designed UTF-8. 2. Plan 9 implemented it. That's not true. UTF-8 was designed, in front of my eyes, on a placemat in a New Jersey diner one night in September or so 1992. What happened was this. We had used the original UTF from ISO 10646 to make Plan 9 support 16-bit characters, but we hated it. We were close to shipping the system when, late one afternoon, I received a call from some folks, I think at IBM - I remember them being in Austin - who were in an X/Open committee meeting. They wanted Ken and me to vet their FSS/UTF design. We understood why they were introducing a new design, and Ken and I suddenly realized there was an opportunity to use our experience to design a really good standard and get the X/Open guys to push it out. We suggested this and the deal was, if we could do it fast, OK. So we went to dinner, Ken figured out the bit-packing, and when we came back to the lab after dinner we called the X/Open guys and explained our scheme. We mailed them an outline of our spec, and they replied saying that it was better than theirs (I don't believe I ever actually saw their proposal; I know I don't remember it) and how fast could we implement it? I think this was a Wednesday night and we promised a complete running system by Monday, which I think was when their big vote was. So that night Ken wrote packing and unpacking code and I started tearing into the C and graphics libraries. The next day all the code was done and we started converting the text files on the system itself. By Friday some time Plan 9 was running, and only running, what would be called UTF-8. We called X/Open and the rest, as they say, is slightly rewritten history. Why didn't we just use their FSS/UTF? As I remember, it was because in that first phone call I sang out a list of desiderata for any such encoding, and FSS/UTF was lacking at least one - the ability to synchronize a byte stream picked up mid-run, with less that one character being consumed before synchronization. Becuase that was lacking, we felt free - and were given freedom - to roll our own. I think the "IBM designed it, Plan 9 implemented it" story originates in RFC2279. At the time, we were so happy UTF-8 was catching on we didn't say anything about the bungled history. Neither of us is at the Labs any more, but I bet there's an e-mail thread in the archive there that would support our story and I might be able to get someone to dig it out. So, full kudos to the X/Open and IBM folks for making the opportunity happen and for pushing it forward, but Ken designed it with me cheering him on, whatever the history books say. -rob ```
cora commented 2022-05-11 15:25:53 +02:00
Contributor
Copy Link

Not sure why that full quote was necessary but the interesting part is what you didn't talk about there I suppose hehe.

@erlehmann complaining here at the 13th hour will get you nothing.

I agree with you about this with the new node stuff but like I said we can't start making a new mod every time something is added or changed. And I actually checked cdb if any mods are using MCL_beds or compass and unsurprisingly none are.

Please try to accept a compromise now and then. It won't get you anything to lose yourself in invalid (and unfair) comparisons like that anyways.

Plus this is a done deal now.

Anyways @kabou pls fix crash MineClone2/MineClone2#2197 (comment)

Not sure why that full quote was necessary but the interesting part is what you didn't talk about there I suppose hehe. @erlehmann complaining here at the 13th hour will get you nothing. I agree with you about this with the new node stuff but like I said we can't start making a new mod every time something is added or changed. And I actually checked cdb if any mods are using MCL_beds or compass and unsurprisingly none are. Please try to accept a compromise now and then. It won't get you anything to lose yourself in invalid (and unfair) comparisons like that anyways. Plus this is a done deal now. Anyways @kabou pls fix crash https://git.minetest.land/MineClone2/MineClone2/pulls/2197#issuecomment-38484
chmodsayshello requested changes 2022-05-11 15:43:46 +02:00
chmodsayshello left a comment
Member
Copy Link

While I agree 99% on your changes, you can stack lodestone compasses in minecraft

While I agree 99% on your changes, you can stack lodestone compasses in minecraft
mods/ITEMS/mcl_compass/init.lua
@ -143,0 +207,4 @@
elseif item.name == "compass_lodestone" then
name_fmt = "mcl_compass:%d_lodestone"
img_fmt = "mcl_compass_compass_%02d.png^[colorize:purple:50"
stack_max = 1
chmodsayshello commented 2022-05-11 15:41:59 +02:00
Member
Copy Link

In minecraft, you CAN stack lodestone compasses, as long as they belong to the same lodestone, but the different metadata would prevent you from doing so

In minecraft, you CAN stack lodestone compasses, as long as they belong to the same lodestone, but the different metadata would prevent you from doing so
kabou commented 2022-05-11 17:13:28 +02:00
Author
Contributor
Copy Link

Oh, I took that bit of information from https://minecraft.fandom.com/wiki/Compass . At the top right, below the images, it says that lodestone compasses are not stackable. I do not have an actual version of mc to cross reference.

Shall I revert this then?

Oh, I took that bit of information from https://minecraft.fandom.com/wiki/Compass . At the top right, below the images, it says that lodestone compasses are not stackable. I do not have an actual version of mc to cross reference. Shall I revert this then?
chmodsayshello commented 2022-05-11 17:47:28 +02:00
Member
Copy Link

@kabou Hmmm, thats strange, but other versions of the wiki actually say that it is stackable, however, I own a copy of Minecraft Java, here is a screenshot taken by me showing it, so yeah, I think this should be reverted...
(I had to add the file to my inital comment because I can't upload files here...)

@kabou Hmmm, thats strange, but other versions of the wiki actually say that it is stackable, however, I own a copy of Minecraft Java, here is a screenshot taken by me showing it, so yeah, I think this should be reverted... (I had to add the file to my inital comment because I can't upload files here...) ![](https://git.minetest.land/attachments/70135f99-89fe-4781-9de3-2b4a007ae43c)
kabou commented 2022-05-11 17:48:53 +02:00
Author
Contributor
Copy Link

Allright, will revert.

Allright, will revert.
kabou commented 2022-05-11 17:56:08 +02:00
Author
Contributor
Copy Link

I have removed the stack_max setting altogether, so it uses the default (which can be altered via settings).

I have removed the `stack_max` setting altogether, so it uses the default (which can be altered via settings).
kabou marked this conversation as resolved
chmodsayshello reviewed 2022-05-11 15:58:55 +02:00
mods/ITEMS/mcl_compass/locale/mcl_compass.de.tr
@ -2,3 +2,3 @@
Compasses are tools which point to the world origin (X@=0, Z@=0) or the spawn point in the Overworld.=Kompasse sind Werkzeuge, die zum Ursprungspunkt der Welt (X@=0, Z@=0) oder zum Einstiegspunkt der Welt zeigen.
Compass=Kompass
Points to the world origin=Zeigt zum Startpunkt der Welt
Compasses are tools which point to the world origin (X@=0, Z@=0) or the spawn point in the Overworld.=Kompasse sind Werkzeuge, die zum Ursprungspunkt der Welt (X@=0, Z@=0) oder zum Einstiegspunkt der Welt zeigen.
chmodsayshello commented 2022-05-11 15:58:55 +02:00
Member
Copy Link

Suggestion for the translation (maybe I should make a seperate PR for that later):

A Compass always points to the world spawn point when the player is in the overworld.  In other dimensions, it spins randomly.=Ein Kompass zeigt immer zum Weltspawn in der Oberwelt. In sämtlichen anderen Dimensionen dreht er sich zufällig.
Lodestone Compass=Leitstein Kompass
Points to a lodestone=Zeigt zu einem Leitstein
Lodestone compasses resemble regular compasses, but they point to a specific lodestone.=Leitstein Kompasse ähneln normalen Kompassen, aber sie zeigen zu einen spezifischen Leitstein.
A Lodestone compass can be made from an ordinary compass by using it on a lodestone.  After becoming a lodestone compass, it always points to its linked lodestone, provided that they are in the same dimension.  If not in the same dimension, the lodestone compass spins randomly, similarly to a regular compass when outside the overworld.  A lodestone compass can be relinked with another lodestone.=Ein Leitstein Kompass kann mit einem normalen Kompass erstellt werden indem man ihn auf einem Leitstein benutzt. Nachdem er ein Leitstein Kompass geworden ist, wird er immer zu seinem Leitstein zeigen, sofern sie in der selben Dimension sind. Wenn sie nicht in der selben Dimension sind, dreht sich der Leitstein Kompass zufällig, wie ein normaler Kompass außerhalb der Oberwelt. Ein Leitstein Kompass kann mit einem anderem Leitstein verknüpft werden.
Suggestion for the translation (maybe I should make a seperate PR for that later): ``` A Compass always points to the world spawn point when the player is in the overworld. In other dimensions, it spins randomly.=Ein Kompass zeigt immer zum Weltspawn in der Oberwelt. In sämtlichen anderen Dimensionen dreht er sich zufällig. Lodestone Compass=Leitstein Kompass Points to a lodestone=Zeigt zu einem Leitstein Lodestone compasses resemble regular compasses, but they point to a specific lodestone.=Leitstein Kompasse ähneln normalen Kompassen, aber sie zeigen zu einen spezifischen Leitstein. A Lodestone compass can be made from an ordinary compass by using it on a lodestone. After becoming a lodestone compass, it always points to its linked lodestone, provided that they are in the same dimension. If not in the same dimension, the lodestone compass spins randomly, similarly to a regular compass when outside the overworld. A lodestone compass can be relinked with another lodestone.=Ein Leitstein Kompass kann mit einem normalen Kompass erstellt werden indem man ihn auf einem Leitstein benutzt. Nachdem er ein Leitstein Kompass geworden ist, wird er immer zu seinem Leitstein zeigen, sofern sie in der selben Dimension sind. Wenn sie nicht in der selben Dimension sind, dreht sich der Leitstein Kompass zufällig, wie ein normaler Kompass außerhalb der Oberwelt. Ein Leitstein Kompass kann mit einem anderem Leitstein verknüpft werden. ```
kabou commented 2022-05-11 17:14:31 +02:00
Author
Contributor
Copy Link

Translation improvements are always good. I'll see if I can add it.

Translation improvements are always good. I'll see if I can add it.
kabou commented 2022-05-11 17:47:39 +02:00
Author
Contributor
Copy Link

Translation added, thanks!

Translation added, thanks!
kabou marked this conversation as resolved
chmodsayshello commented 2022-05-11 16:06:59 +02:00
Member
Copy Link

Why exactly not put the lodestone stuff in a separate mod?

If you want to look at it like this, it's my fault: I made the PR to add lodestones in mineclone5's mcl_beds, and when I've created the very same PR for mcl2, i just used mcl_beds again so you can switch between mineclone 2 & 5 without too much stuff breaking, and that PR got merged, so unless we move the entire code to somewhere else (which wouldn't make sense in my opinion since it depends on mcl_compass while literally being a compass), it won't be in a separate mod, this PR just improves the existing code a bit

> Why exactly not put the lodestone stuff in a *separate* mod? If you want to look at it like this, it's my fault: I made the PR to add lodestones in mineclone5's mcl_beds, and when I've created the very same PR for mcl2, i just used mcl_beds again so you can switch between mineclone 2 & 5 without too much stuff breaking, and that PR got merged, so unless we move the entire code to somewhere else (which wouldn't make sense in my opinion since it depends on mcl_compass while literally being a compass), it won't be in a separate mod, this PR just improves the existing code a bit
erlehmann commented 2022-05-11 16:33:52 +02:00
Member
Copy Link

@erlehmann complaining here at the 13th hour will get you nothing.

As I already wrote you privately, I have deduced that it is almost impossible to convince people that their design is bad, even if the alternative is super simple.

I actually checked cdb if any mods are using MCL_beds or compass and unsurprisingly none are.

For the question if something is good or bad design, these kind of things are almost entirely irrelevant. IMO good design does not accidentally hinder use cases not yet envisioned – while bad design does.

A big difference between good and bad software architects is that one can figure out how to design software that can accomodate as-of-yet-unknown use cases easily in the future, while the other will claim “you can not know what you want in the future” is a reason to never even think about that.

Whatever, I give up. Clearly any effort I make to prevent entire categories of bugs is dwarfed by the sounds of LALALA I CAN NOT HEAR YOU & IT IS WAY TOO LATE TO SPEAK UP NOW.

> @erlehmann complaining here at the 13th hour will get you nothing. As I already wrote you privately, I have deduced that it is almost impossible to convince people that their design is bad, even if the alternative is super simple. > I actually checked cdb if any mods are using MCL_beds or compass and unsurprisingly none are. For the question if something is good or bad design, these kind of things are almost entirely irrelevant. IMO good design does not accidentally hinder use cases not yet envisioned – while bad design does. A big difference between good and bad software architects is that one can figure out how to design software that can accomodate as-of-yet-unknown use cases easily in the future, while the other will claim “you can not know what you want in the future” is a reason to never even think about that. Whatever, I give up. Clearly any effort I make to prevent entire categories of bugs is dwarfed by the sounds of LALALA I CAN NOT HEAR YOU & IT IS WAY TOO LATE TO SPEAK UP NOW.
cora commented 2022-05-11 16:37:28 +02:00
Contributor
Copy Link

As I also already told you privately (not sure why that needs to be mentioned but OK) if you would be less extremist and less rude about it you would probably have better chances getting what you want.

As I also already told you privately (not sure why that needs to be mentioned but OK) if you would be less extremist and less rude about it you would probably have better chances getting what you want.
erlehmann commented 2022-05-11 16:41:46 +02:00
Member
Copy Link

As I also already told you privately (not sure why that needs to be mentioned but OK) if you would be less extremist and less rude about it you would probably have better chances getting what you want.

And you are probably correct about that. I just do not have the patience anymore.

> As I also already told you privately (not sure why that needs to be mentioned but OK) if you would be less extremist and less rude about it you would probably have better chances getting what you want. And you are probably correct about that. I just do not have the patience anymore.
kabou commented 2022-05-11 17:15:38 +02:00
Author
Contributor
Copy Link

crash in 5.4 when rightclicking lode stone:

mods/ITEMS/mcl_compass/init.lua:104: attempt to call upvalue 'vec_from_str' (a nil value)

why not use minetest.string_to_pos / pos_to_string like it did before ?

Oops, got bitten by 5.5 vector-fu a second time. Thanks for spotting that. Will fix now.

> crash in 5.4 when rightclicking lode stone: > > `mods/ITEMS/mcl_compass/init.lua:104: attempt to call upvalue 'vec_from_str' (a nil value)` > > why not use minetest.string_to_pos / pos_to_string like it did before ? Oops, got bitten by 5.5 vector-fu a second time. Thanks for spotting that. Will fix now.
kabou commented 2022-05-11 17:26:43 +02:00
Author
Contributor
Copy Link

@erlehmann complaining here at the 13th hour will get you nothing.

As I already wrote you privately, I have deduced that it is almost impossible to convince people that their design is bad, even if the alternative is super simple.

There are many factors to consider in software design. One of those is code sharing. By putting every little thing into its own mod, we'll need even more mods for the code shared by those mods.

Since the main use case that you keep coming up with for rationalizing mod proliferation is some (largely mythical) external mods that moreover can claim absolute primacy over compatibility considerations, your position is not always perceived as reasonable by the people who try to develop mineclone as an integrated game.

Whatever, I give up. Clearly any effort I make to prevent entire categories of bugs is dwarfed by the sounds of LALALA I CAN NOT HEAR YOU & IT IS WAY TOO LATE TO SPEAK UP NOW.

So I pretty much refuted your arguments in that other thread by pointing out that you cannot process utf-8 in EDLIN.COM and you merrily ignored that. Have a look.

> > @erlehmann complaining here at the 13th hour will get you nothing. > > As I already wrote you privately, I have deduced that it is almost impossible to convince people that their design is bad, even if the alternative is super simple. There are many factors to consider in software design. One of those is code sharing. By putting every little thing into its own mod, we'll need even more mods for the code shared by those mods. Since the main use case that you keep coming up with for rationalizing mod proliferation is some (largely mythical) external mods that moreover can claim absolute primacy over compatibility considerations, your position is not always perceived as reasonable by the people who try to develop mineclone as an integrated game. > Whatever, I give up. Clearly any effort I make to prevent entire categories of bugs is dwarfed by the sounds of LALALA I CAN NOT HEAR YOU & IT IS WAY TOO LATE TO SPEAK UP NOW. So I pretty much refuted your arguments in that other thread by pointing out that you cannot process utf-8 in EDLIN.COM and you merrily ignored that. [Have a look](https://en.wikipedia.org/wiki/Mirror).
kabou added 2 commits 2022-05-11 17:45:25 +02:00
bacc7613b5 Fix crash in mt 5.4 with vector ops. 
* `vector.from_string()` is not available in mt pre-5.5.  Replace with
  `minetest.string_to_pos()`.
aca4aca79b Add German translation. 
* Add "de" (German) translation by chmodsayshello.
kabou commented 2022-05-11 17:46:59 +02:00
Author
Contributor
Copy Link

@cora I fixed the crash and added the translation. I'm not yet sure about the stacking of lodestone compasses. @kneekoo do you have an opinion on that? [edit: in any case I did revert it anyway.]

@cora I fixed the crash and added the translation. I'm not yet sure about the stacking of lodestone compasses. @kneekoo do you have an opinion on that? [edit: in any case I did revert it anyway.]
kabou added 1 commit 2022-05-11 17:55:20 +02:00
8ae605165b Fix lodestone compass stack_max. 
* Lodestone compasses are stackable.
* Remove hardcoded `stack_max` setting, use default.
chmodsayshello commented 2022-05-11 18:06:12 +02:00
Member
Copy Link

Sorry, but it looks like I have to request changes again :(

Sorry, but it looks like I have to request changes again :(
chmodsayshello requested changes 2022-05-11 18:07:46 +02:00
chmodsayshello left a comment
Member
Copy Link

Lodestone and normal compasses are not supposed to have the same groups

Lodestone and normal compasses are not supposed to have the same groups
mods/ITEMS/mcl_compass/init.lua
@ -172,0 +217,4 @@
_tt_help = item.tt,
inventory_image = string.format(img_fmt, i),
wield_image = string.format(img_fmt, i),
groups = {compass = i + 1, tool = 1, disable_repair = 1},
chmodsayshello commented 2022-05-11 18:07:06 +02:00
Member
Copy Link

This applies these groups to every compass, no matter if it's a normal or lodestone one.
Lodestone Compasses are not supposed to be in the creative inventory, you need to check what kind of compass it is and set not_in_creative_inventory to 0 or 1 (0 is default)

This applies these groups to every compass, no matter if it's a normal or lodestone one. Lodestone Compasses are not supposed to be in the creative inventory, you need to check what kind of compass it is and set `not_in_creative_inventory` to 0 or 1 (0 is default)
kabou commented 2022-05-11 20:12:23 +02:00
Author
Contributor
Copy Link

I think that's what it does? (if not set explicitly to 1, it defaults to 0.)
8ae605165b/mods/ITEMS/mcl_compass/init.lua (L222-L228)

if i == stereotype_frame then
	def._doc_items_longdesc = item.longdesc
	def._doc_items_usagehelp = item.usagehelp
else
	def._doc_items_create_entry = false
	def.groups.not_in_creative_inventory = 1
end
I think that's what it does? (if not set explicitly to 1, it defaults to 0.) https://git.minetest.land/MineClone2/MineClone2/src/commit/8ae605165b5ce7e87136b565cec6763782d41083/mods/ITEMS/mcl_compass/init.lua#L222-L228 ```lua if i == stereotype_frame then def._doc_items_longdesc = item.longdesc def._doc_items_usagehelp = item.usagehelp else def._doc_items_create_entry = false def.groups.not_in_creative_inventory = 1 end ```
kabou commented 2022-05-11 20:14:26 +02:00
Author
Contributor
Copy Link

Oh wait, I see what you mean, lodestone compasses are not supposed to be in creative inventory at all..

Oh wait, I see what you mean, lodestone compasses are not supposed to be in creative inventory at all..
kabou commented 2022-05-11 20:21:11 +02:00
Author
Contributor
Copy Link

I'm not sure that's a big problem really.

A lodestone compass in creative inventory will not have an associated lodestone, so it will be like a lodestone compass that has lost its association.

I can easily change the condition into if i == stereotype_frame and item == "compass" then but first I'd like to hear some other people's opinion what should be appropriate here.

I'm not sure that's a big problem really. A lodestone compass in creative inventory will not have an associated lodestone, so it will be like a lodestone compass that has lost its association. I can easily change the condition into `if i == stereotype_frame and item == "compass" then` but first I'd like to hear some other people's opinion what should be appropriate here.
cora commented 2022-05-11 20:22:26 +02:00
Contributor
Copy Link

O yeah I wondered about that. But the ls-compass without meta turns back to normal compass which is good anyways

O yeah I wondered about that. But the ls-compass without meta turns back to normal compass which is good anyways
kabou commented 2022-05-11 20:27:38 +02:00
Author
Contributor
Copy Link

I don't think it does, they have different itemnames.

I don't think it does, they have different itemnames.
cora commented 2022-05-11 20:28:35 +02:00
Contributor
Copy Link

Try

Try
kabou commented 2022-05-11 20:43:24 +02:00
Author
Contributor
Copy Link

Indeed. But this is another problem, unrelated to the craftitem definitions. The globalstep code reverts any lodestone compass that has empty meta to a regular compass.

Indeed. But this is another problem, unrelated to the craftitem definitions. The globalstep code reverts any lodestone compass that has empty meta to a regular compass.
kabou commented 2022-05-11 21:51:13 +02:00
Author
Contributor
Copy Link

The latest commits should fix this issue. I had to overhaul some logic to not rely on the item meta, but to instead string.find the itemname.

This also required the lodestone on_rightclick to be changed.

The latest commits should fix this issue. I had to overhaul some logic to not rely on the item meta, but to instead string.find the itemname. This also required the lodestone on_rightclick to be changed.
chmodsayshello commented 2022-05-12 18:42:01 +02:00
Member
Copy Link

But they are still in the creative inventory, and that's the "real" problem in my opinion, since they have no meta and no lodestone to point to... I would suggest doing it like in this commit (based on the current state of your branch)

But they are still in the creative inventory, and that's the "real" problem in my opinion, since they have no meta and no lodestone to point to... I would suggest doing it like in this [commit (based on the current state of your branch)](https://git.minetest.land/chmodsayshello/MineClone2/commit/94e3a0d8128cfcf32b04d7e0ade3353b23292aaa)
kabou commented 2022-05-12 19:57:34 +02:00
Author
Contributor
Copy Link

Personally I don't mind having unset lodestone compasses in creative inventory, as I said before, they behave similarly to lodestone compasses that lost their lodestone. But if the consensus is that they should not be in creative inventory, then that is not a problem for me at all.

Personally I don't mind having unset lodestone compasses in creative inventory, as I said before, they behave similarly to lodestone compasses that lost their lodestone. But if the consensus is that they should not be in creative inventory, then that is not a problem for me at all.
chmodsayshello commented 2022-05-12 21:28:31 +02:00
Member
Copy Link

personally, I don't like adding commits to other people's PRs, but cora added my commit which removes them from the creative inventory, so we can close this one

personally, I don't like adding commits to other people's PRs, but cora added my commit which removes them from the creative inventory, so we can close this one
👍 1
kabou commented 2022-05-12 21:32:02 +02:00
Author
Contributor
Copy Link

Allright, now if you can "approve" the review, it'll look nicer when cora merges..

Allright, now if you can "approve" the review, it'll look nicer when cora merges..
chmodsayshello marked this conversation as resolved
kabou added 3 commits 2022-05-11 21:46:04 +02:00
14c882f982 Fix lodestone compass meta handling. 
* The nature of a compass was being determined by looking at its meta.
  This caused lodestone compasses with unset meta to turn into regular
  compasses.  Fixed by using string matching on the itemname.
* Changed lodestone rightclick handler to explicitly set the correct
  name and frame of the compass used on it instead of waiting for
  globalstep to do this.
74e70b674e Fix return value of `get_compass_image()`. 
* `get_compass_image()` did not actually return the image number.
872b708465 Remove unused variable. 
* Removed unused variable `stack_max`.
cora approved these changes 2022-05-12 15:14:43 +02:00
cora left a comment
Contributor
Copy Link

Works fine now. @chmodsayshello you happy with it now ?

Works fine now. @chmodsayshello you happy with it now ?
chmodsayshello commented 2022-05-12 18:38:03 +02:00
Member
Copy Link

Works fine now. @chmodsayshello you happy with it now ?

Actually, not 100%, while this PR changes the code in a "good" way, I really think that lodestone compasses shouldn't be in the creative inventory, I made a copy of this branch in my old mcl2 fork, I would suggest doing it like in this commit

> Works fine now. @chmodsayshello you happy with it now ? Actually, not 100%, while this PR changes the code in a "good" way, I really think that lodestone compasses shouldn't be in the creative inventory, I made a copy of this branch in my old mcl2 fork, I would suggest doing it like in this [commit](https://git.minetest.land/chmodsayshello/MineClone2/commit/94e3a0d8128cfcf32b04d7e0ade3353b23292aaa)
👍 2
chmodsayshello commented 2022-05-12 18:49:49 +02:00
Member
Copy Link

Another problem: Once you drop a lodestone compass now, it turns into a normal one

Another problem: Once you drop a lodestone compass now, it turns into a normal one
cora added 1 commit 2022-05-12 19:06:31 +02:00
cora commented 2022-05-12 19:07:16 +02:00
Contributor
Copy Link

how about this ^^

you can do that too btw.

check out
https://git.minetest.land/MineClone2/MineClone2/wiki/Best-git-tricks

to learn most of my git-fu hehe

how about this ^^ you can do that too btw. check out https://git.minetest.land/MineClone2/MineClone2/wiki/Best-git-tricks to learn most of my git-fu hehe
cora commented 2022-05-12 19:17:19 +02:00
Contributor
Copy Link

Another problem: Once you drop a lodestone compass now, it turns into a normal one

mmmh that is indeed a problem with this mod then. metadata transfer to item entities works fine with other stuff like enchanted tools or written books.

> Another problem: Once you drop a lodestone compass now, it turns into a normal one mmmh that is indeed a problem with this mod then. metadata transfer to item entities works fine with other stuff like enchanted tools or written books.
kabou commented 2022-05-12 20:02:17 +02:00
Author
Contributor
Copy Link

There is no longer any logic that uses metadata to determine the type of compass, it is all based on item name now. But that happens only with itemstacks that are in a player's inventory.

I'm a bit stumped about what's going on here (and I also still don't have my head straight atm.)

There is no longer any logic that uses metadata to determine the type of compass, it is all based on item name now. But that happens only with itemstacks that are in a player's inventory. I'm a bit stumped about what's going on here (and I also still don't have my head straight atm.)
cora commented 2022-05-12 20:11:25 +02:00
Contributor
Copy Link

Uh i dont think it matters a lot but it doesnt really make sense to have lodestone compass in creative imo

Uh i dont think it matters a lot but it doesnt really make sense to have lodestone compass in creative imo
kabou commented 2022-05-12 20:31:43 +02:00
Author
Contributor
Copy Link

No problem. But I don't understand the item change on drop. I added some debug statements to the only place in code (globalstep) where itemname is set, but it doesn't seem to hit that when dropping an item.

No problem. But I don't understand the item change on drop. I added some debug statements to the only place in code (globalstep) where itemname is set, but it doesn't seem to hit that when dropping an item.
MysticTempest commented 2022-05-12 20:38:26 +02:00
Member
Copy Link

Another problem: Once you drop a lodestone compass now, it turns into a normal one

No problem. But I don't understand the item change on drop. I added some debug statements to the only place in code (globalstep) where itemname is set, but it doesn't seem to hit that when dropping an item.

@chmodsayshello @kabou

Looks this happens due to the custom item drop code for compasses in 'mcl_item_entity':
https://git.minetest.land/MineClone2/MineClone2/src/branch/get-compass-image-fix/mods/ENTITIES/mcl_item_entity/init.lua#L420-L421

Patching that section with this seems to work:

		if minetest.get_item_group(stack:get_name(), "mcl_compass:compass") > 0 then
			stack:set_name("mcl_compass:16")
			itemstring = stack:to_string()
			self.itemstring = itemstring
		elseif minetest.get_item_group(stack:get_name(), "mcl_compass:compass_lodestone") > 0 then
			stack:set_name("mcl_compass_lodestone:16")
			itemstring = stack:to_string()
			self.itemstring = itemstring
		end
> Another problem: Once you drop a lodestone compass now, it turns into a normal one > No problem. But I don't understand the item change on drop. I added some debug statements to the only place in code (globalstep) where itemname is set, but it doesn't seem to hit that when dropping an item. @chmodsayshello @kabou Looks this happens due to the custom item drop code for compasses in 'mcl_item_entity': https://git.minetest.land/MineClone2/MineClone2/src/branch/get-compass-image-fix/mods/ENTITIES/mcl_item_entity/init.lua#L420-L421 Patching that section with this seems to work: ``` if minetest.get_item_group(stack:get_name(), "mcl_compass:compass") > 0 then stack:set_name("mcl_compass:16") itemstring = stack:to_string() self.itemstring = itemstring elseif minetest.get_item_group(stack:get_name(), "mcl_compass:compass_lodestone") > 0 then stack:set_name("mcl_compass_lodestone:16") itemstring = stack:to_string() self.itemstring = itemstring end ```
kabou commented 2022-05-12 20:49:25 +02:00
Author
Contributor
Copy Link

Ooh thanks a lot for pointing that out.

Ooh thanks a lot for pointing that out.
cora commented 2022-05-12 21:02:46 +02:00
Contributor
Copy Link

LOL the meta stays properly it just sets the name to normal compass - this shouldnt be too hard to fix

LOL the meta stays properly it just sets the name to normal compass - this shouldnt be too hard to fix
cora commented 2022-05-12 21:04:16 +02:00
Contributor
Copy Link

get this. if i make a normal compass with "pointsto" meta with that method on this branch, then switch back to master and relog it turns back to lodestone compass and works

looks like @chmodsayshello has made a rock solid system hehe

get this. if i make a normal compass with "pointsto" meta with that method on this branch, then switch back to master and relog it turns back to lodestone compass and works looks like @chmodsayshello has made a rock solid system hehe
cora commented 2022-05-12 21:06:51 +02:00
Contributor
Copy Link

oh didnt see that last commit ... works now <3

oh didnt see that last commit ... works now <3
kabou force-pushed get-compass-image-fix from 1603d0bacd to 85fe5baf7a 2022-05-12 21:07:05 +02:00 Compare
kabou force-pushed get-compass-image-fix from 85fe5baf7a to baf8e0b79c 2022-05-12 21:09:09 +02:00 Compare
kabou commented 2022-05-12 21:12:32 +02:00
Author
Contributor
Copy Link

Sorry, I tweaked the last commit a bit and force pushed the amended commit.

I used a slightly different approach from what @MysticTempest suggested, namely:

if minetest.get_item_group(stack:get_name(), "compass") > 0 then
	if string.find(stack:get_name(), "_lodestone") then
		stack:set_name("mcl_compass:18_lodestone")
	else
		stack:set_name("mcl_compass:18")
	end
	itemstring = stack:to_string()
	self.itemstring = itemstring
end
Sorry, I tweaked the last commit a bit and force pushed the amended commit. I used a slightly different approach from what @MysticTempest suggested, namely: ```lua if minetest.get_item_group(stack:get_name(), "compass") > 0 then if string.find(stack:get_name(), "_lodestone") then stack:set_name("mcl_compass:18_lodestone") else stack:set_name("mcl_compass:18") end itemstring = stack:to_string() self.itemstring = itemstring end ```
👍 1
chmodsayshello commented 2022-05-12 21:17:12 +02:00
Member
Copy Link

get this. if i make a normal compass with "pointsto" meta with that method on this branch, then switch back to master and relog it turns back to lodestone compass and works

looks like @chmodsayshello has made a rock solid system hehe

lol, I thought obvious, since in the current master code, it checks if there is meta on the item, and then uses it if there is a lodestone

> get this. if i make a normal compass with "pointsto" meta with that method on this branch, then switch back to master and relog it turns back to lodestone compass and works > > looks like @chmodsayshello has made a rock solid system hehe lol, I thought obvious, since in the current master code, it checks if there is meta on the item, and then uses it if there is a lodestone
kabou commented 2022-05-12 21:18:41 +02:00
Author
Contributor
Copy Link

BTW, I tested a bit without the special case code in mcl_item_entity altogether and it works equally well. The difference is that the dropped (lodestone) compass is left in the state (direction) it was when dropped, instead of always pointing in the stereotype direction.

Maybe I even like it better like that.

BTW, I tested a bit without the special case code in mcl_item_entity altogether and it works equally well. The difference is that the dropped (lodestone) compass is left in the state (direction) it was when dropped, instead of always pointing in the stereotype direction. Maybe I even like it better like that.
cora commented 2022-05-12 21:19:45 +02:00
Contributor
Copy Link

well i never looked at the code in depth, just thought "wow this reacts in a very sane way to a completely fabricated situation"

well i never looked at the code in depth, just thought "wow this reacts in a very sane way to a completely fabricated situation"
cora commented 2022-05-12 21:21:02 +02:00
Contributor
Copy Link

BTW, I tested a bit without the special case code in mcl_item_entity altogether and it works equally well. The difference is that the dropped (lodestone) compass is left in the state (direction) it was when dropped, instead of always pointing in the stereotype direction.

Maybe I even like it better like that.

it makes 0 practical difference though since it would immediately be updated when in player inv again.

so yeah I think we can merge this now !?

> BTW, I tested a bit without the special case code in mcl_item_entity altogether and it works equally well. The difference is that the dropped (lodestone) compass is left in the state (direction) it was when dropped, instead of always pointing in the stereotype direction. > > Maybe I even like it better like that. it makes 0 practical difference though since it would immediately be updated when in player inv again. so yeah I think we can merge this now !?
kabou commented 2022-05-12 21:25:04 +02:00
Author
Contributor
Copy Link

get this. if i make a normal compass with "pointsto" meta with that method on this branch, then switch back to master and relog it turns back to lodestone compass and works

looks like @chmodsayshello has made a rock solid system hehe

On master, if you drop a lodestone compass, it also becomes a compass item entity, but when you pick it up again, it reverts to the lodestone compass by action of the globalstep code.

> get this. if i make a normal compass with "pointsto" meta with that method on this branch, then switch back to master and relog it turns back to lodestone compass and works > > looks like @chmodsayshello has made a rock solid system hehe On master, if you drop a lodestone compass, it also becomes a compass item entity, but when you pick it up again, it reverts to the lodestone compass by action of the globalstep code.
kabou commented 2022-05-12 21:29:40 +02:00
Author
Contributor
Copy Link

it makes 0 practical difference though since it would immediately be updated when in player inv again.

One big difference is that we'd get rid of the compass specific case in mcl_item_entity. This could better be discussed in a different issue or pr.

so yeah I think we can merge this now !?

If it doesn't crash anymore and it behaves like it should - okay.

> it makes 0 practical difference though since it would immediately be updated when in player inv again. One big difference is that we'd get rid of the compass specific case in mcl_item_entity. This could better be discussed in a different issue or pr. > so yeah I think we can merge this now !? If it doesn't crash anymore and it behaves like it should - okay.
chmodsayshello commented 2022-05-12 21:36:48 +02:00
Member
Copy Link

This time my testing was not so intensive (around 7 mins), but I found no issues, and the code is clean and understandable

This time my testing was not so intensive (around 7 mins), but I found no issues, and the code is clean and understandable
chmodsayshello approved these changes 2022-05-12 21:37:00 +02:00
cora merged commit 729159f631 into master 2022-05-12 23:37:18 +02:00
cora deleted branch get-compass-image-fix 2022-05-12 23:37:24 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
MysticTempest
AFCMS
NO11
erlehmann
cora
chmodsayshello
Labels
Clear labels   
#P1 CRITICAL

for crashes, catastrophic bugs, etc.

  
#P2: HIGH

highest priority level

  
#P3: elevated

2nd-highest priority level

  
#P4 priority: medium

for things that aren't elevated but have been prioritised

  
#P6: low

low priority level

  
#Review

Tickets that may improve or fix an issue but will need a review after a short period of time

  
annoying

non-bugs that annoys people

  
API

   
bug

   
code quality

   
combat

   
commands

   
compatibility

Label to cover migration issues from old worlds or mcl5

  
configurability

   
contribution inside

   
controls

   
core feature

   
creative mode

   
delayed for engine release

   
documentation

   
duplicate

   
enhancement

enhancement request/suggestion

  
environment

Issues that affect weather or light

  
gameplay

   
graphics

   
ground content conflict

mapgen gc-specific quirks

  
GUI/HUD

   
help wanted

   
incomplete feature

   
invalid / won't fix

Invalid, or will not fix

  
items

   
looking for contributor

   
mapgen

   
meta

Relating to VoxeLibre on Mesehub and elsewhere

  
mineclone2+

fit for MineClone2-Plus

  
Minecraft >= 1.13

   
Minecraft >= 1.17

   
missing feature

   
mobile

involving Minetest on mobile devices (android)

  
mobs

   
mod support

Issues related to mod support

  
model needed

A model is needed to be created or updated

  
multiplayer

Issues affecting multiplayer

  
Needs adoption

PR needs author action or to be adopted so changes can be implemented to get the PR closer to merge

  
needs discussion

   
needs engine change

   
needs more information

   
needs research

   
nodes

   
non-Minecraft feature

   
non-mob entities

   
performance

   
player

issues related to the player

  
possible close

   
redstone

   
release notes

This ticket will need information adding to release notes

  
schematics

   
Skyblock

   
sounds

   
Testing / Retest

This needs testing or retesting

  
tools

Related to the tools scripts, which aren't run in game

  
translation

   
unconfirmed
  
mcl5

Possibly relevant for MineClone5

  
mcla

Possibly relevant for Mineclonia

  
Media missing

media files for this feature are missing - contributions welcome

No Label
#P1 CRITICAL
#P2: HIGH
#P3: elevated
#P4 priority: medium
#P6: low
#Review
annoying
API
bug
code quality
combat
commands
compatibility
configurability
contribution inside
controls
core feature
creative mode
delayed for engine release
documentation
duplicate
enhancement
environment
gameplay
graphics
ground content conflict
GUI/HUD
help wanted
incomplete feature
invalid / won't fix
items
looking for contributor
mapgen
meta
mineclone2+
Minecraft >= 1.13
Minecraft >= 1.17
missing feature
mobile
mobs
mod support
model needed
multiplayer
Needs adoption
needs discussion
needs engine change
needs more information
needs research
nodes
non-Minecraft feature
non-mob entities
performance
player
possible close
redstone
release notes
schematics
Skyblock
sounds
Testing / Retest
tools
translation
unconfirmed
mcl5
mcla
Media missing
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: VoxeLibre/VoxeLibre#2197
No description provided.
Delete Branch "get-compass-image-fix"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?