Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MineClone/MineClonia compatibility for technic core #328

Merged
merged 83 commits into from
Jan 10, 2024

Conversation

nonfreegithub
Copy link
Contributor

@nonfreegithub nonfreegithub commented Sep 30, 2023

  • sounds
  • dig groups
  • mcl_blast_resistance
  • mcl_hardness
  • crafts
  • formspec
  • recipes
  • mcl_craftguide support

@nonfreegithub nonfreegithub marked this pull request as draft September 30, 2023 14:55
@BuckarooBanzay BuckarooBanzay added Compatibility WIP Work in progress labels Sep 30, 2023
@corarona
Copy link

very nice, tested a bit, seems to work well so far

@corarona
Copy link

corarona commented Oct 24, 2023

One possible issue: HV-quarry can dig bedrock ... and void

@corarona
Copy link

screenshot_20231024_235030

@S-S-X
Copy link
Member

S-S-X commented Oct 25, 2023

Have to add sounds to fixtures for tests, I can also add that if you want?

- mcl_core:bedrock
- mcl_core:barrier
- df_underworld_items:slade
- df_underworld_items:slade_brick
- df_underworld_items:slade_wall
- df_underworld_items:slade_sand
- df_underworld_items:slade_block
- df_underworld_items:slade_seal
@S-S-X
Copy link
Member

S-S-X commented Oct 25, 2023

how can I fix the lua check failed?

Luacheck is passing, just mineunit failing. That's exactly what requires sounds to mineunit fixtures, I'll see where it should be added, check if anything else is needed and hopefully add commit to fix tests shortly.

@nonfreegithub
Copy link
Contributor Author

I ran into an issue: there is no way to get a low voltage transformer: making it requires technic:rubber (replacement for fiber), but to make technic:rubber requires coal dust, which requires a grinding, which requires lv power, which requires a switching station, which requires a low voltage transformer again.

fixed

Alternatively, a more expensive (or less efficient) alloy recipe could be added, such as 2x latex + 1x coal lump = 2x rubber.

alloy recipe added

@BuckarooBanzay
Copy link
Member

@OgelGames you self-requested a review, do you need more time (no pressure:) to look at this or can we get this merged and sort out potential issues later? (looked good last time i tested it)

@S-S-X
Copy link
Member

S-S-X commented Dec 23, 2023

@OgelGames you self-requested a review, do you need more time (no pressure:) to look at this or can we get this merged and sort out potential issues later? (looked good last time i tested it)

I got few change requests but most of it already good and will approve after luacheck fix and tools/init.lua reset.

Copy link
Member

@S-S-X S-S-X left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change request requires comment, just adding to make it clear

technic/tools/init.lua Outdated Show resolved Hide resolved
technic/tools/init.lua Outdated Show resolved Hide resolved
@OgelGames
Copy link
Contributor

OgelGames commented Dec 24, 2023

@OgelGames you self-requested a review, do you need more time (no pressure:) to look at this or can we get this merged and sort out potential issues later? (looked good last time i tested it)

Yeah, I wanted to look over all the code one more time, because I've missed a lot of the commits. I also wanted to test it a bit. I know it's probably fine, but with such a big PR, it's good to triple check it :)

Copy link
Member

@S-S-X S-S-X left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but have not actually tested in game. Currently it focuses pretty well on MCL compatibility while still keeping changes fairly simple and clear 👍

technic/init.lua Outdated Show resolved Hide resolved
technic/machines/HV/nuclear_reactor.lua Outdated Show resolved Hide resolved
technic/materials.lua Outdated Show resolved Hide resolved
@BuckarooBanzay
Copy link
Member

i resolved some conflicts from the merged PR in the master branch, ignore the mtinfo workflow error when merging, will remove that feature in another PR i think (it caused more trouble than it is worth)

@nonfreegithub
Copy link
Contributor Author

why the check fail?

@BuckarooBanzay
Copy link
Member

why the check fail?

don't worry about the mtinfo check, it currently fails because the test still uses the 5.3.0 docker image which doesn't have the vector.zero() function

@BuckarooBanzay
Copy link
Member

i don't see anything blocking this PR anymore, all comments are resolved and we have 3 approvals, will test this one more time and merge within one or 2 days if there are no objections (cleanups and minor fixes can be done in a separate PR)

@BuckarooBanzay BuckarooBanzay merged commit da0378a into mt-mods:master Jan 10, 2024
5 checks passed
@Athozus Athozus added this to the 2.0.0 milestone Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants