search

SHsuperCM / CITResewn

Fabric implementation of mcpatcher's cit
MIT License
146 stars 72 forks source link

Make CITResewn work on 1.20.4 (maybe) breaking old versions :) #353

Closed Dev0Louis closed 3 months ago

Dev0Louis commented 7 months ago

I did testing and nothing seems to be broken. The only thing that changed is that we need to capture the field now and make it a field our self.

CommandGenius commented 7 months ago

could you make a built version if that's fine?

Cryodev1 commented 6 months ago

Crashes whenever launching for reason "Mixin transformation of net.minecraft.class_4599 failed" Seems to be because BufferBuilderStorage requires an argument of int maxBlockBuildersPoolSize, this doesn't seem to implement that. Why doesn't it? Beyond me, never worked in mods before. Not sure where to set it, but this seems to be the problem.

Edit: This seems to be specific to 1.20.3+ meaning the old versions will in fact break with a version that works on the new updates

TechMasterApp commented 6 months ago

I think you want to update to 1.20.4 while you are at it...

dicedpixels commented 6 months ago

If anyone's interested, I published my personal build for 1.20.4 here: https://github.com/dicedpixels/CITResewn/releases/tag/1.20.4-1.1.4-dicedpixels. Everything should work.

Crystal-Spider commented 6 months ago

If anyone's interested, I published my personal build for 1.20.4 here: https://github.com/dicedpixels/CITResewn/releases/tag/1.20.4-1.1.4-dicedpixels. Everything should work.

This seems to work perfectly, would be nice if you opened a PR and it got merged. It would also close #358

BenignPigeon commented 6 months ago

@dicedpixels please could you open a PR? It would also close #361

dicedpixels commented 6 months ago

@BenignPigeon I'd rather wait for the author. Even if I make a PR, the author needs to merge it. For now, my build should suffice.

Crystal-Spider commented 6 months ago

@dicedpixels but for the author it might be easier to merge a PR rather than update manually the code.
But anyway I agree that still we need to wait for the author in both cases, just maybe with the PR it might be faster.

BenignPigeon commented 6 months ago

Completely see your point dicedpixels and i think Nyphet has a good point too it it will probably be easier for the author to merge changes rather than have to do it manually

Dev0Louis commented 6 months ago

Oh hey I just saw this, wait let me fix this real quick

DovydasTEDS commented 5 months ago

@SHsuperCM any thought about this? Sry for the mention.

lucksducks commented 5 months ago

Emissive CIT not working on Handheld items

dicedpixels commented 5 months ago

Emissive CIT not working on Handheld items

Did it work on older versions? I don't think emissives were ever a part of CITR.

https://github.com/SHsuperCM/CITResewn/issues/123#issuecomment-1077023538

lucksducks commented 5 months ago

maybe theyre not part of it, but it worked on 1.20/1.20.1

dicedpixels commented 5 months ago

maybe theyre not part of it, but it worked on 1.20/1.20.1

Can you provide a resource pack? I want to double check.

SHsuperCM commented 5 months ago

@lucksducks Emissive textures is not and never was something CITR implemented. If you had emissive textures then another mod added it.

Apologies for this *very*\ over-extended delay with this update, I'll try to take a look soon.

Crystal-Spider commented 5 months ago

Any update on this? I'd like to release a 1.20.4 modpack and this is the only mod missing the update

Crystal-Spider commented 5 months ago

@SHsuperCM , sorry for the ping, could I at least get your permission to ship @dicedpixels 's build in my modpack please? I guess I might need @dicedpixels permission too.

dicedpixels commented 5 months ago

@SHsuperCM , sorry for the ping, could I at least get your permission to ship @dicedpixels 's build in my modpack please? I guess I might need @dicedpixels permission too.

Go ahead. Please give credit to the original author. My fork is just a few changes to get it working on 1.20.4.

HowardZHY commented 5 months ago

@lucksducks Emissive textures is not and never was something CITR implemented. If you had emissive textures then another mod added it.

Apologies for this _*very*_ over-extended delay with this update, I'll try to take a look soon.

Please merge other PRs as well when merging this

HowardZHY commented 4 months ago

@SHsuperCM , sorry for the ping, could I at least get your permission to ship @dicedpixels 's build in my modpack please? I guess I might need @dicedpixels permission too.

Go ahead. Please give credit to the original author. My fork is just a few changes to get it working on 1.20.4.

Can you merge other PR's fixes as well for your 1.20.4 build?

dicedpixels commented 4 months ago

@SHsuperCM , sorry for the ping, could I at least get your permission to ship @dicedpixels 's build in my modpack please? I guess I might need @dicedpixels permission too.

Go ahead. Please give credit to the original author. My fork is just a few changes to get it working on 1.20.4.

Can you merge other PR's fixes as well for your 1.20.4 build?

Will have to think about this. My knowledge of the mod's internal workings is limited and I'd rather not merge anything that's not approved by the original author.

HowardZHY commented 4 months ago

@SHsuperCM , sorry for the ping, could I at least get your permission to ship @dicedpixels 's build in my modpack please? I guess I might need @dicedpixels permission too.

Go ahead. Please give credit to the original author. My fork is just a few changes to get it working on 1.20.4.

Can you merge other PR's fixes as well for your 1.20.4 build?

Will have to think about this. My knowledge of the mod's internal workings is limited and I'd rather not merge anything that's not approved by the original author.

Can you make an 1.20.4 build with the JSON Lag Patch?

dicedpixels commented 4 months ago

@SHsuperCM , sorry for the ping, could I at least get your permission to ship @dicedpixels 's build in my modpack please? I guess I might need @dicedpixels permission too.

Go ahead. Please give credit to the original author. My fork is just a few changes to get it working on 1.20.4.

Can you merge other PR's fixes as well for your 1.20.4 build?

Will have to think about this. My knowledge of the mod's internal workings is limited and I'd rather not merge anything that's not approved by the original author.

Can you make an 1.20.4 build with the JSON Lag Patch?

I cherry-picked that PR a while back and found out it broke some RPs. I'd like to keep the current version as stable as possible.

Crystal-Spider commented 4 months ago

Hi @SHsuperCM , any update on this? As I said before, could you please either release a 1.20.4 version or give me permission to include dicepixels' build into my modpack?
Thanks

DovydasTEDS commented 4 months ago

The project is open source and licensed under the MIT license. This means you don't need permission to use the mod or fork.

DovydasTEDS commented 4 months ago

@Crystal-Spider

The project is open source and licensed under the MIT license. This means you don't need permission to use the mod or fork.

Permission is provided through the MIT license. I personally used this mod in my minecraft modpack TEDS Plus

SHsuperCM commented 3 months ago

Superseded by upstream. This PR would've broken other 1.20 versions as well which is part of what was stopping it from being merged. As a note for the future as well, try to keep the impact of PRs you make as small as possible, this PR breaks a few things that it shouldnt've even come in contact with.

@Crystal-Spider As others said, I made CIT Resewn use an open license for a reason. everyone is free to do whatever they wish with the source code. (ofc I'd appreciate credits regardless)

SHsuperCM commented 3 months ago

A 1.20.4 release will come soon as I check a few things before publishing.