cc-tweaked / cc-restitched

Other
80 stars 33 forks source link

Merge upstream #135

Closed SquidDev closed 1 year ago

SquidDev commented 1 year ago

Yeah, I don't really know how to write a description for this. This pulls in the work on making CC:T multi-loader compatible, which should make it easier to keep CC:R in sync with CC:T in the future.

Fixes #121, Fixes #122, Closes #133. Should also resolve some of the other issues (#130), but I'm less confident there.

Merith-TK commented 1 year ago

I assume this is just for 1.19 and wont be expecting an 1.18 equivalent? I would like to keep 1.18 CC:R maintained atleast until 1.20 is released if possible...

SquidDev commented 1 year ago

I assume this is just for 1.19 and wont be expecting an 1.18 equivalent?

Correct. It's got a lot of API breaking changes in (hence doing it now, after 1.19.3 has already broken everything), so it's not really viable to backport any of this.

Merith-TK commented 1 year ago

I assume this has been somewhat tested on fabric/quilt?

SquidDev commented 1 year ago

Yep. All the game tests now run on Fabric too, but I've a friend running this on a private server for a few weeks. I'm sure there are some bugs which haven't surfaced, but it should be pretty solid.

Merith-TK commented 1 year ago

GH Actions have not produced an build, nor can I build in Gitpod, if you could look into that, I gotta head to work atm.

SquidDev commented 1 year ago

Fixed! I forgot to update the path to the upload task in GH.

Patbox commented 1 year ago

Looking that CC:T's repo also has fabric related code now, does it make sense to maintain it in two repos?

Other thing is it looks like it lost support for Common Protection API on fabric

Merith-TK commented 1 year ago

Currently with CC:R's goal of maintaining parity, merging the repos into one repo for development doesn't seem like that bad of an idea.

Problem is I don't know if the dev environments are 100% compatible between the two (for example. Can one compile the fabric code from the current forge repo or vice-versa?)

If we can manage that then merging the repos into one seems like an acceptable possibility

On Sun, Dec 11, 2022, 1:50 PM Patbox @.***> wrote:

Looking that CC:T's repo also has fabric related code now, does it make sense to maintain it in two repos?

Other thing is it looks like it lost support for Common Protection API on fabric

— Reply to this email directly, view it on GitHub https://github.com/cc-tweaked/cc-restitched/pull/135#issuecomment-1345666103, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACPQOXTVQUOZL4MULTWS5IDWMZEDFANCNFSM6AAAAAAS263YYA . You are receiving this because you commented.Message ID: @.***>

Patbox commented 1 year ago

It should from what I've quickly checked the repo, since it is some multiloader setup (similar to one used by botania for example)

SquidDev commented 1 year ago

Looking that CC:T's repo also has fabric related code now, does it make sense to maintain it in two repos?

While I would definitely appreciate bug fixes being submitted upstream, I still think it makes sense for CC:R to be its own project. CC:R has got its own changes (i.e. new textures!) and general "feel" which I think merit it being its own thing.

The multi-loader work was very much done to make it easier to keep the two mods in sync (especially in the API, which has diverged over the last few MC versions), rather than merge the mods themselves.

Other thing is it looks like it lost support for Common Protection API on fabric

We now fire all the appropriate Fabric interaction events, so this should work by default with claim mods. I'd also expect #114 and #115 to be fixed, though I haven't checked yet.

Merith-TK commented 1 year ago

"Add back the netherite pickaxe"

Ah yes, the tool that got added on accident and serves no purpose other than to flex that you have enough netherite that you can spend it on an turtle. And don't forget it was netherite tools, plural lol

Honestly I was think if the mods got merged into one repo, you could do what we have done for the classic textures. But for the fabric. And just label them "Restitched" and "Tweaked" respectively, and default to either depending on modloader.

On Sun, Dec 11, 2022, 3:28 PM Jonathan Coates @.***> wrote:

Looking that CC:T's repo also has fabric related code now, does it make sense to maintain it in two repos?

While I would definitely appreciate bug fixes being submitted upstream, I still think it makes sense for CC:R to be its own project. CC:R has got its own changes (i.e. new textures!) and general "feel" which I think merit its own thing.

The multi-loader work was very much done to make it easier to keep the two mods in sync (especially in the API, which has diverged over the last few MC versions), rather than merge the mods themselves.

Other thing is it looks like it lost support for Common Protection API on fabric

We now fire all the appropriate Fabric interaction events, so this should work by default with claim mods. I'd also expect #114 https://github.com/cc-tweaked/cc-restitched/issues/114 and #115 https://github.com/cc-tweaked/cc-restitched/issues/115 to be fixed, though I haven't checked yet.

— Reply to this email directly, view it on GitHub https://github.com/cc-tweaked/cc-restitched/pull/135#issuecomment-1345688706, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACPQOXQDJXTLGAA7HB3PE3TWMZPQZANCNFSM6AAAAAAS263YYA . You are receiving this because you commented.Message ID: @.***>

Patbox commented 1 year ago

Yep that sounds like good solution

Merith-TK commented 1 year ago

I see you made a datapack, the main reason why I was saying the specific tools that CC:R has is because in all regards except netherite tool's on turtles, you can take an world that only has CC:T/R on it, and take it to the other loader, so worlds that use computercraft for map making (and create worlds) can be ported over to fabric or forge without issue outside of loader specific issues with the mods (more recently this should be fixed in both mods stated)

SquidDev commented 1 year ago

Ahh, that was in response to https://github.com/cc-tweaked/CC-Tweaked/issues/1254 rather than CC:R specifically.

SquidDev commented 1 year ago

I think this PR is stalled but, as people are using CC:T for Fabric more than I'd like, would quite like to get some resolution here.

The main problem as it currently stands is that CC:T for Fabric and CC:R's APIs are incompatible, due to drift over the last year. While worlds are compatible between the two - peripheral mods which target one will crash on the other.

I think there's a matrix of options here:

Merith-TK commented 1 year ago

Personally since CC:T is officially going multiloader fabric. I am for depreciation of CC:R in favor of CC:T, especially with the API Drift due to the main goal of CC:R being 1:1 parity with CC:T, (Outside of textures and the aforementioned Netherite Tool turtles. Of which I'm still trying to figure out when the fuck those got added)

Patbox commented 1 year ago

Yeah, deprecating CC:R makes sense. Through without unpublishing of current ver and only with a "redirect"

Patbox commented 1 year ago

Alternatively we can wait for 1.19.4, but who knows when it releases

SquidDev commented 1 year ago

Alternatively we can wait for 1.19.4, but who knows when it releases

Yep, happy to go that route!

SquidDev commented 1 year ago

As agreed above, CC:T for 1.19.4 publishes Fabric jars as well as Forge ones (see https://github.com/cc-tweaked/CC-Tweaked/commit/266182996da07d9b4535479e1b6b896ff8682820), so closing this.

Merith-TK commented 1 year ago

honestly having one final release for CC:R (an 1.18.2 and 1.19.2/3 release) of the latest update wouldn't be a bad idea in my opinion, would allow resolving a few final bugs.

assuming CC:T's official fabric work wont get backported that is. if it does get backported that would also be great

SquidDev commented 1 year ago

assuming CC:T's official fabric work wont get backported

Yeah, correct. The multi-loader work was a massive piece of work, I have no wish to do it again :).

Merith-TK commented 1 year ago

I mean in theory it should be as simple as backporting the code a bit, same way you would get normal code to back port but, I can see why one would say no on that front :)