Skip to content

Conversation

@nixnoxus
Copy link
Contributor

This PR adds wrench support for connected_chests (https://github.com/HybridDog/connected_chests)

@OgelGames
Copy link
Contributor

IMO this should be done from another mod, not here, that's why the API exists.

@nixnoxus
Copy link
Contributor Author

nixnoxus commented Nov 28, 2021

IMO this should be done from another mod, not here, that's why the API exists.

i understand the objection a little. but why are digtron and drawers supported here?

@OgelGames
Copy link
Contributor

i understand the objection a little. but why are digtron and drawers supported here?

drawers is there from before the API was functional, and the support had to be kept for compatibility. digtron is there because other compatibility code already existed. (though that may change in the future)

@S-S-X @BuckarooBanzay your opinions?

@S-S-X
Copy link
Member

S-S-X commented Nov 28, 2021

@S-S-X @BuckarooBanzay your opinions?

For tools like wrench my opinion is that it could very well add support within wrench mod.
There upsides and downsides with both approaches.

For wrench itself I do think that it should not be part of technic modpack at all but that discussion probably does not belong here, mentioned because it is related: adding more stuff that is not directly related with technic itself is not good.
This point only holds while wrench is included in technic modpack, afaik there's no plans to change this but it might be good thing to do.

Important thing when using API (assuming it is stable) is that it allows other mod to easily add/update/remove fields and internal data if there's changes in mod, no need to remember another pull request for wrench.
Also when there's multiple mods released with same name (rewrites and forks that add something or remove something) then there's compatibility to consider: having implementation in mod allows that exact mod to control how tool behaves, having implementation in tool mod makes behavior same for all mods.

Having a lot of support implemented in wrench itself allows keeping other mods cleaner and all wrench (or any tool) related stuff stays with tool itself. From PoV of another mod having optional dependencies and compatibility code for many small tools is not nice or very clean especially with simple mods.
When tool does already override other mods then having actual mod support implementation included with tool makes bit more sense, wrench does this and through that wrench is also kind of responsible of compatibility.

No clear opinion, it depends but upsides of using API should be considered.

@nixnoxus
Copy link
Contributor Author

the background of my question:

i am currently working on further wrench support for

  • bones:bones
  • xdecor: ..
  • pipeworks:autocrafter,deployer,dispenser

possibly more later. Originally i wanted to make a PR for every supported mod here

@OgelGames
Copy link
Contributor

Maybe a separate mod should be created, just for adding support for other mods? (wrench_nodes maybe?)

I was also thinking of adding support for mods on the Pandorabox server (in its custom mod), but it could be better to put it in a separate mod, so others can use it.

@nixnoxus
Copy link
Contributor Author

i don't think it's a good idea to create and maintain a third mod that mediates between wrench and supported mods. this new mod would not add any fundamentally new functionality to the game.
also maintaining this mod would be a very thankless task. in german we call this: 'sitting between two chairs'.

i think the better way would be, as it was suggested by s-x-x, to split off the wrench from technic and then add and maintain the support for the other mods there.

@S-S-X
Copy link
Member

S-S-X commented Nov 29, 2021

I'll create repo for wrench, we'll see how it looks after mangling all the history. Decisions about actually moving can be made later but if it looks good could also be immediately effective.
AFAIK wrench does not depend on anything in technic modpack and technic modpack does not depend on anything provided by wrench.

@S-S-X
Copy link
Member

S-S-X commented Nov 29, 2021

See https://github.com/mt-mods/wrench and https://github.com/mt-mods/wrench/commits/master if it looks fine...
Branches and tags probably should be removed completely unless there's specific reason to keep some, if so then those tags/branches should be rewritten to follow new history.

@OgelGames
Copy link
Contributor

OgelGames commented Nov 29, 2021

I'd say delete all the tags and branches, and we'll add the first tag/release when we release technic 2.0.0.

I'm amazed that you were able to make the commit history so clean 👀

AFAIK wrench does not depend on anything in technic modpack and technic modpack does not depend on anything provided by wrench.

Only the craft recipe setting, which can (and should) be changed:

if minetest.get_modpath("technic") and technic.config:get_bool("enable_wrench_crafting") then

@S-S-X
Copy link
Member

S-S-X commented Nov 29, 2021

Only the craft recipe setting, which can (and should) be changed:

For compatibility it would be possible to check for craft recipe configuration from technic mod (safe because of optional dependency for machines) but maybe better to just drop it.

Mod anyway has to be reinstalled from new location and in my opinion this also allows changing (and should be changed) default for craft recipe to be enabled, however being very useful admin tool for hard survival servers config should still be there to allow continuing its use as admin tool.

Basically I think most of "has to be done" stuff would be:

@nixnoxus
Copy link
Contributor Author

AFAIK wrench does not depend on anything in technic modpack and technic modpack does not depend on anything provided by wrench.

Only the craft recipe setting, which can (and should) be changed:

if minetest.get_modpath("technic") and technic.config:get_bool("enable_wrench_crafting") then

I have adjusted this in mt-mods/wrench@1cbc803#diff-631a6c0f99ae783007a3e649fe10e409a1dba08ef54b4a35d21c7ef086fc7911R28

my current state of development can be seen in mt-mods/wrench#1

@S-S-X
Copy link
Member

S-S-X commented Nov 30, 2021

@nixnoxus Sorry about trouble again, there still one thing I did not think about while deciding to nuke it from orbit instead of just force pushing:
Recreating repository after cleaning up history detached your fork and therefore pull requests cannot be directly made.

So options are:

  • Recreate fork and apply move your work there OR
  • Work directly on mt-mods/wrench, I've added invite with write access and created
    branch based on your work wrench_support_extend, accept invite and you have direct write access.

@S-S-X S-S-X mentioned this pull request Nov 30, 2021
5 tasks
@BuckarooBanzay
Copy link
Member

@S-S-X @BuckarooBanzay your opinions?

too late to the party, sorry, but IMO: a standalone wrench is probably best in the long run (it is headed into that direction anyway from the looks of it...)

@S-S-X S-S-X added the Won't fix This will not be worked on label Apr 22, 2022
@S-S-X
Copy link
Member

S-S-X commented Apr 22, 2022

Not needed anymore / belongs to https://github.com/mt-mods/wrench

@S-S-X S-S-X closed this Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Won't fix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants