Mineclonia
/
Mineclonia
Archived
12
17
Fork 13
Code Issues 105 Pull Requests 19 Projects Releases Wiki Activity

mcl_inventory: Remove _mcl_autogroup dependency from mcl_inventory #76

Merged
cora merged 2 commits from fix_inventory_depends into master 2021-06-25 17:25:46 +02:00
Conversation 15 Commits 2 Files Changed 2 +5 -6
ryvnf commented 2021-05-23 20:51:13 +02:00
Owner
Copy Link
Problem

TRACKING ISSUE: #67

mcl_inventory has a dependency on _mcl_autogroup which should be avoided for all mods in Mineclonia. It uses this to compute item categories for creative inventory.

Solution

This is solved by moving the code into minetest.register_on_mods_loaded.

Details

We cannot simply remove the _mcl_autogroup dependency because the code needs to be executed after all items have been registered.

Testing Steps
The inventories does not change
  1. Go into creative mode
  2. Check that the creative inventory has the expected number of items for each of the categories:
    • Building blocks: 8 pages, 14 items on last page
    • Decoration blocks: 5 pages, 44 items on last page
    • Redstone: 2 pages, 11 items on last page
    • Transportation: 1 page, 16 items on last page
    • Brewing: 4 pages, 2 items on last page
    • Miscellaneous: 1 page, 19 items on last page
    • Foodstuffs: 1 page, 38 items on page
    • Tools: 1 page, 36 items on last page
    • Combat: 2 pages, 37 items on last page
    • Mobs: 2 pages, 1 item on last page
    • Materials: 2 pages, 13 items on last page
_mcl_autogroup is loaded as the last mod
  1. Switch to master branch using git checkout master
  2. Run minetest -v |& grep Mod | grep loaded on the master branch
  3. Verify that _mcl_autogroup is not loaded last
  4. Switch to the PR branch using git checkout fix_inventory_depends
  5. Run minetest -v |& grep Mod | grep loaded on the master branch
  6. Verify that _mcl_autogroup is loaded last
##### Problem TRACKING ISSUE: #67 `mcl_inventory` has a dependency on `_mcl_autogroup` which should be avoided for all mods in Mineclonia. It uses this to compute item categories for creative inventory. ##### Solution This is solved by moving the code into `minetest.register_on_mods_loaded`. ##### Details We cannot simply remove the `_mcl_autogroup` dependency because the code needs to be executed after all items have been registered. ##### Testing Steps ###### The inventories does not change 1. Go into creative mode 2. Check that the creative inventory has the expected number of items for each of the categories: - Building blocks: 8 pages, 14 items on last page - Decoration blocks: 5 pages, 44 items on last page - Redstone: 2 pages, 11 items on last page - Transportation: 1 page, 16 items on last page - Brewing: 4 pages, 2 items on last page - Miscellaneous: 1 page, 19 items on last page - Foodstuffs: 1 page, 38 items on page - Tools: 1 page, 36 items on last page - Combat: 2 pages, 37 items on last page - Mobs: 2 pages, 1 item on last page - Materials: 2 pages, 13 items on last page ###### `_mcl_autogroup` is loaded as the last mod 1. Switch to master branch using `git checkout master` 2. Run `minetest -v |& grep Mod | grep loaded` on the master branch 3. Verify that `_mcl_autogroup` is not loaded last 4. Switch to the PR branch using `git checkout fix_inventory_depends` 5. Run `minetest -v |& grep Mod | grep loaded` on the master branch 6. Verify that `_mcl_autogroup` is loaded last
ryvnf added 1 commit 2021-05-23 20:51:15 +02:00
4b4d40f158 Use register_on_mods_loaded in mcl_inventory 
This removes the need to include _mcl_autogroup as a dependency for
mcl_inventory (which should be avoided) and decreases the likelihood
that the code used for populating item tables is executed before all
other mods are loaded.
ryvnf added the
code quality
 label 2021-05-23 20:54:01 +02:00
ryvnf changed title from mcl_inventory: Use register_on_mods_loaded in mcl_inventory to mcl_inventory: Remove mcl_autogroup dependency from mcl_inventory 2021-05-23 22:59:05 +02:00
ryvnf changed title from mcl_inventory: Remove mcl_autogroup dependency from mcl_inventory to mcl_inventory: Remove _mcl_autogroup dependency from mcl_inventory 2021-05-23 22:59:12 +02:00
ryvnf requested review from Members 2021-06-19 19:20:47 +02:00
ryvnf commented 2021-06-19 19:21:30 +02:00
Author
Owner
Copy Link

Can this PR get some love?

Can this PR get some love?
erlehmann commented 2021-06-22 11:31:51 +02:00
Owner
Copy Link

Can this PR get some love?

Sorry for the delay.

How does this PR affect survival mode?

> Can this PR get some love? Sorry for the delay. How does this PR affect survival mode?
erlehmann commented 2021-06-22 11:39:27 +02:00
Owner
Copy Link

@ryvnf would this change affect the availability of items from third party mods in the creative inventory?

@ryvnf would this change affect the availability of items from third party mods in the creative inventory?
ryvnf commented 2021-06-22 12:11:35 +02:00
Author
Owner
Copy Link

How does this PR affect survival mode?

It should not have any effect.

@ryvnf would this change affect the availability of items from third party mods in the creative inventory?

No.

> How does this PR affect survival mode? It should not have any effect. > @ryvnf would this change affect the availability of items from third party mods in the creative inventory? No.
erlehmann commented 2021-06-22 13:28:58 +02:00
Owner
Copy Link

Thanks. Please include testing steps to verify that this does not break anything in survival or regarding third party mod items.

Thanks. Please include testing steps to verify that this does not break anything in survival or regarding third party mod items.
ryvnf commented 2021-06-22 19:49:14 +02:00
Author
Owner
Copy Link

Thanks. Please include testing steps to verify that this does not break anything in survival or regarding third party mod items.

What makes you think something could break in survival? Or regarding third party mod items?

> Thanks. Please include testing steps to verify that this does not break anything in survival or regarding third party mod items. What makes you think something could break in survival? Or regarding third party mod items?
erlehmann commented 2021-06-22 21:51:46 +02:00
Owner
Copy Link

What makes you think something could break in survival?

I remember the last time mod dependecies changed and it broke survival.

Or regarding third party mod items?

Basically, I would like to know: If a third party mod somehow manages to register items, are they listed correctly in the creative inventory?

> What makes you think something could break in survival? I remember the last time mod dependecies changed and it broke survival. > Or regarding third party mod items? Basically, I would like to know: If a third party mod somehow manages to register items, are they listed correctly in the creative inventory?
ryvnf commented 2021-06-23 13:10:34 +02:00
Author
Owner
Copy Link

I remember the last time mod dependecies changed and it broke survival.

Added steps to verify that _mcl_autogroup is the PR does what it is supposed to and _mcl_autogroup is loaded last. That also tests that it does not break digging like what happend previously.

Basically, I would like to know: If a third party mod somehow manages to register items, are they listed correctly in the creative inventory?

I will not make a test cases for this. There is no logical reason that this would break third party mods, as the minetest.register_on_mods_loaded callback will only be called after loading all mods. Internal and external mods are not loaded differently.

> I remember the last time mod dependecies changed and it broke survival. Added steps to verify that `_mcl_autogroup` is the PR does what it is supposed to and `_mcl_autogroup` is loaded last. That also tests that it does not break digging like what happend previously. > Basically, I would like to know: If a third party mod somehow manages to register items, are they listed correctly in the creative inventory? I will not make a test cases for this. There is no logical reason that this would break third party mods, as the `minetest.register_on_mods_loaded` callback will only be called after loading all mods. Internal and external mods are not loaded differently.
erlehmann commented 2021-06-23 13:57:23 +02:00
Owner
Copy Link

Run minetest |& grep Mod | grep loaded on the master branch

Which shell should be used for that? I have never seen the |& syntax before.

(I am pretty sure it is not Bourne Shell, though.)

> Run minetest |& grep Mod | grep loaded on the master branch Which shell should be used for that? I have never seen the `|&` syntax before. (I am pretty sure it is not Bourne Shell, though.)
ryvnf commented 2021-06-23 14:26:10 +02:00
Author
Owner
Copy Link

Which shell should be used for that? I have never seen the |& syntax before.

It's Bash syntax (but think it works in some other shells too). It is equivalent to 2>&1 |.

> Which shell should be used for that? I have never seen the |& syntax before. It's Bash syntax (but think it works in some other shells too). It is equivalent to `2>&1 |`.
cora commented 2021-06-25 13:17:41 +02:00
Member
Copy Link

uhhm .. i just started testing this, it's probably a typo / not a big deal but please correct this in your testing steps:

Decoration blocks: 5 pages, 48 items on last page

there can not possibly be 48 items on a 9x5 page - it is 44 for me (one is missing from being full on the last page.

Tools: 1 page, 36 items on last page

This needs to be updated to 37 – the screwdrivers has been added to creative inv since (pr #99).

uhhm .. i just started testing this, it's probably a typo / not a big deal but please correct this in your testing steps: > Decoration blocks: 5 pages, 48 items on last page there can not possibly be 48 items on a 9x5 page - it is 44 for me (one is missing from being full on the last page. >Tools: 1 page, 36 items on last page This needs to be updated to 37 – the screwdrivers has been added to creative inv since (pr #99).
cora commented 2021-06-25 13:20:53 +02:00
Member
Copy Link

minetest |& grep Mod | grep loaded

Also i think it needs to be minetest --verbose. It doesnt show anything for me otherwise.

> minetest |& grep Mod | grep loaded Also i think it needs to be minetest --verbose. It doesnt show anything for me otherwise.
cora commented 2021-06-25 13:23:10 +02:00
Member
Copy Link

And it would also be good to specify on which exact commits something needs to be tested.

And it would also be good to specify on which exact commits something needs to be tested.
cora commented 2021-06-25 13:25:25 +02:00
Member
Copy Link

otherwise all tests worked out.

otherwise all tests worked out.
ryvnf commented 2021-06-25 16:07:18 +02:00
Author
Owner
Copy Link

Also i think it needs to be minetest --verbose. It doesnt show anything for me otherwise.

Yes. Forgot to specify that. Nice you figured it out.

there can not possibly be 48 items on a 9x5 page - it is 44 for me (one is missing from being full on the last page.

Yes. I did the math wrong when counting the number of cells.

@cora Can you approve the PR so it can be merged?

> Also i think it needs to be minetest --verbose. It doesnt show anything for me otherwise. Yes. Forgot to specify that. Nice you figured it out. > there can not possibly be 48 items on a 9x5 page - it is 44 for me (one is missing from being full on the last page. Yes. I did the math wrong when counting the number of cells. @cora Can you approve the PR so it can be merged?
cora added 1 commit 2021-06-25 17:12:12 +02:00
cora approved these changes 2021-06-25 17:25:16 +02:00
cora left a comment
Member

Testing Steps
The inventories does not change

  • Switch to master branch using git checkout master
  • Run minetest -v |& grep Mod | grep loaded on the master branch
  • Verify that _mcl_autogroup is not loaded last
  • Switch to the PR branch using git checkout fix_inventory_depends
  • Run minetest -v |& grep Mod | grep loaded on the fixed branch
  • Verify that _mcl_autogroup is loaded last

I did these in both branches:

  • Go into creative mode
  • Check that the creative inventory has the expected number of items for each of the categories:-
  • Building blocks: 8 pages, 14 items on last page
  • Decoration blocks: 5 pages, 44 items on last page
  • Redstone: 2 pages, 11 items on last page
  • Transportation: 1 page, 16 items on last page
  • Brewing: 4 pages, 2 items on last page
  • Miscellaneous: 1 page, 19 items on last page
  • Foodstuffs: 1 page, 38 items on page
  • Tools: 1 page, 36 items on last page
    there's 37 items on the last page now since screwdriver was added to creative inv in pr #99
  • Combat: 2 pages, 37 items on last page
  • Mobs: 2 pages, 1 item on last page
  • Materials: 2 pages, 13 items on last page

I also verified that basic game actions still work in survival.

Since the only test that failed is the screwdriver thing and the changes seem simple enough i will approve it. Even though the testing instructions could have been clearer.

Testing Steps The inventories does not change - [x] Switch to master branch using git checkout master - [x] Run minetest -v |& grep Mod | grep loaded on the master branch - [x] Verify that _mcl_autogroup is not loaded last - [x] Switch to the PR branch using git checkout fix_inventory_depends - [x] Run minetest -v |& grep Mod | grep loaded on the fixed branch - [x] Verify that _mcl_autogroup is loaded last I did these in both branches: - [x] Go into creative mode - [x] Check that the creative inventory has the expected number of items for each of the categories:- [x] - [x] Building blocks: 8 pages, 14 items on last page - [x] Decoration blocks: 5 pages, 44 items on last page - [x] Redstone: 2 pages, 11 items on last page - [x] Transportation: 1 page, 16 items on last page - [x] Brewing: 4 pages, 2 items on last page - [x] Miscellaneous: 1 page, 19 items on last page - [x] Foodstuffs: 1 page, 38 items on page - [ ] Tools: 1 page, 36 items on last page *there's 37 items on the last page now since screwdriver was added to creative inv in pr #99* - [x] Combat: 2 pages, 37 items on last page - [x] Mobs: 2 pages, 1 item on last page - [x] Materials: 2 pages, 13 items on last page I also verified that basic game actions still work in survival. Since the only test that failed is the screwdriver thing and the changes seem simple enough i will approve it. Even though the testing instructions could have been clearer.
cora merged commit 926d5e2c37 into master 2021-06-25 17:25:46 +02:00
cora deleted branch fix_inventory_depends 2021-06-25 17:26:14 +02:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
cora
Labels
Clear labels   
blocker

blocks release until resolved

  
bug

   
code quality

   
confirmed

   
critical

crashes, catastrophic bugs

  
discussion

   
high priority

   
incompatibility

incompatible with mcl2

  
incomplete feature

   
invalid

   
low priority

   
missing feauture

   
needs testing

   
packet spam

   
performance

   
project

meta-issues

  
regression

   
translations

   
unconfirmed
  
in review

   
ready for review
No Label
blocker
bug
code quality
confirmed
critical
discussion
high priority
incompatibility
incomplete feature
invalid
low priority
missing feauture
needs testing
packet spam
performance
project
regression
translations
unconfirmed
in review
ready for review
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
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: Mineclonia/Mineclonia#76
No description provided.
Delete Branch "fix_inventory_depends"

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?