minetest-mods / 3d_armor

Visible player armor & wielded items for minetest
Other
17 stars 39 forks source link

improved load print patch #96

Closed Zweihorn closed 1 year ago

Zweihorn commented 1 year ago

From the start my prior PR was aiming at compatibility with legacy clients and servers. If you scan the MT forum you will become aware that there seem to be quite many MT 0.4 servers around which are actively used by many players.

However, only after commiting that PR #95 another project kindly made me aware of the minetest.log function, which presumably is a somewhat newer MT core dev invention. Accordingly, the best solution may be this example of a new piece of improved code, if I understand the MT Lua code correctly.

-- print to log after mod was loaded successfully
local load_message = "[MOD] XXX loaded"
if minetest.log then
    minetest.log("info", load_message) -- aims at state of the art MT software
else
    print (load_message)  -- aims at legacy MT software used in the field
end

Furthermore, all the 17 files are comprised by one commit now, after I took the opportunity to (again) install the GitHub Desktop App i.e. program on my computer.

Hope this helps.

fluxionary commented 1 year ago
  1. minetest.log has been around since at least the 0.4.0 release. i'm not sure what the minimum version of minetest that 3d_armor supports, but it's something a lot newer than that (e.g. it needs the minetest.settings object).
  2. the success and amount of time it takes to load a mod is already part of the info log-level stream. set debug_log_level = info in minetest.conf, or pass the --info argument to minetest when you launch it.
Zweihorn commented 1 year ago
  1. minetest.log has been around since at least the 0.4.0 release. i'm not sure what the minimum version of minetest that 3d_armor supports, but it's something a lot newer than that (e.g. it needs the minetest.settings object).
  2. the success and amount of time it takes to load a mod is already part of the info log-level stream. set debug_log_level = info in minetest.conf, or pass the --info argument to minetest when you launch it.

Thanks for the update. I am quite neutral on that as it is basically a nice opportunity to make myself again familiar with the GitHub environment and the MT community in particualr. I am always happy to learn.

However, I may have missed the main entry point but (by some tradition, I fear) even today there seem to be too much contradictory sources of information available and digging into the MT forum is not an easy task. I would appreciate some more accessibility and quality control on the MT core documentation (the core code itself seems to be excellent), the Lua API, the apparently quite outdated MT wiki and would fear of a lack of information in general. "Just look into the code on GitHub" shouldn't be the default.

Or do I go wrong and the (unofficial) Modding Handbook nowadays would provide an up-to-date comprisal?

Zweihorn commented 1 year ago

My sincere apologies for my mishap and naivety. The condition of most of the external sources may be a nightmare but obviously the "Just look into the MT core code on GitHub" should be the default.

  1. minetest.log is well documented in the lua_api.txt of the stable-0.4 branch i.e. the Minetest Lua Modding API Reference 0.4.17
  2. In additon I found the Man-Page template minetest.6 in the doc folder of the MT core. For reasons (yet) unknown the build I am using was not producing the man-pages and I found no external information in advance.

Some lessons learned and a good training on the job, I presume.

BuckarooBanzay commented 1 year ago

I'm very sorry to be the buzzkill here:

IMO: the manual mod-log-statements always annoyed me in the mods that have it, it is basically dead code without added value.

(i'm open for discussion though)

For you own sanity: can you please open an issue next time before you spend time implementing something, i hate closing PR's of people that invested some time into it...

Zweihorn commented 1 year ago

For you own sanity: can you please open an issue next time before you spend time implementing something, i hate closing PR's of people that invested some time into it...

You are welcome abd thanks for your time. No problem at all and the time invested from my side was at my deliberation. This is free and open software in a community of volunteers.

Personally I take it as a good learning experience and an opportunity for a treining on the job. The job being making myself more involved with the MT code and MT mods and the MT comunity.

Last not least and on top of that is my personal gain and the goal in becoming self-trained on the tool chain after some rather unfortunate longer break.

Zweihorn commented 1 year ago

No harm done hopefully. Besides that it is quite interesting to me how much difference one can observe apparently between the opinions of the MT core devs and long-standing MT modders with practical multi-player MT server admin experience.

Anyway from a technical point of view I should have stayed with #95 as this PR (#96) is not optimal at all and just overkill. The minetest.log call would be quite redundant due to the apparent INFO log-level defaults.

THX and closed

Zweihorn commented 1 year ago

BTW

I learned a new word today:

buzzkill | ˈbʌzkɪl | noun North American informal a person or thing that has a depressing or dispiriting effect:

@BuckarooBanzay - You are welcome. No spirit was harmed in the making and closing of this PR.

BuckarooBanzay commented 1 year ago

thanks for understanding, if you have more questions/ideas you can always head over on irc or the discord server 👍