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

migrate to quilt enigma #268

Merged
merged 9 commits into from
Jun 23, 2024
Merged

Conversation

ix0rai
Copy link
Contributor

@ix0rai ix0rai commented Mar 3, 2024

gaming

the stats stuff isn't particularly useful as of the latest version, it throws up on quite a few parchment classes. I'm going to work on improving that

@CLAassistant
Copy link

CLAassistant commented Mar 3, 2024

CLA assistant check
All committers have signed the CLA.

@ix0rai
Copy link
Contributor Author

ix0rai commented Mar 3, 2024

stat issues tracked by QuiltMC/enigma#184. from a little bit of poking around, that's the only major bug affecting parchment

@NebelNidas
Copy link

NebelNidas commented Mar 5, 2024

It might be advantageous to keep Fabric's one around as an option, in case unintended issues arise. Kinda like Loom, which provides both genSourcesWithCfr and genSourcesWithVineflower, with the standard genSources defaulting to one of the aforementioned ones.

@ix0rai
Copy link
Contributor Author

ix0rai commented Mar 5, 2024

that's not really an option if the parchment project begins using a plugin for automatic mapping

@marchermans
Copy link
Member

Can somebody explain to me why this pr was made?

The pr description is really bad: "gaming"?!? What has gaming to do with this? Also what is up with the stats?

Second of al, if this is a fork of fabrics enigma then what indicates zo us that the quilt variant will stay with upstream / is better then fabrics?

@NebelNidas
Copy link

It's no secret that Fabric's Enigma has been falling behind in features lately, Quilt's fork has added quite a few quality of life improvements (more powerful plugin system, UI indicators for mapping progress, fixing of the hosted server etc). Although I'd argue that most of these aren't relevant for Parchment, except maybe for the Vineflower integration. Which Fabric is only missing because the required API hasn't been released yet, and we don't want to depend on unstable snapshot versions.

So while Fabric's fork has been stale for quite a while now, Quilt's changes don't provide much benefit for Parchment (at least as of now), and I too am currently working on a large internal refactor for Fabric's Enigma, aiming to improve stability and extensibility in the long run. That's why I proposed to keep both options for now, as long as you don't start developing custom Enigma plugins, there shouldn't be any drawbacks

@marchermans
Copy link
Member

It's no secret that Fabric's Enigma has been falling behind in features lately, Quilt's fork has added quite a few quality of life improvements (more powerful plugin system, UI indicators for mapping progress, fixing of the hosted server etc). Although I'd argue that most of these aren't relevant for Parchment, except maybe for the Vineflower integration. Which Fabric is only missing because the required API hasn't been released yet, and we don't want to depend on unstable snapshot versions.

So while Fabric's fork has been stale for quite a while now, Quilt's changes don't provide much benefit for Parchment (at least as of now), and I too am currently working on a large internal refactor for Fabric's Enigma, aiming to improve stability and extensibility in the long run. That's why I proposed to keep both options for now, as long as you don't start developing custom Enigma plugins, there shouldn't be any drawbacks

So are you a maintainer of Fabrics Enigma, or Quilts?

@NebelNidas
Copy link

NebelNidas commented Mar 25, 2024

I'm working on Fabric's one, since Quilt's fork had horrible performance regressions in the past which made it unusable for my personal deobfuscation project. Afaik there has been work on resolving these, though I haven't conducted any new tests in the last few months

@marchermans
Copy link
Member

@ix0rai Could you provide me with a list of advantages of the Quilt Enigma version vs the Fabric one. Also what are its disadvantages?

@ix0rai
Copy link
Contributor Author

ix0rai commented Mar 25, 2024

I didn't include a big description, because when I made this PR I thought the decision was already made based on my conversations with the maintainers, specifically @sciwhiz12. Either way, here are some key features for parchment:

  • as mentioned, the added Vineflower decompiler produces what is IMO leagues better output than anything else
  • our docker system allows you to reorganise and hide tabs such as the class tree, the implementations view, and others, to make more room for your code or display more information
  • the statistic icons in the class tree provide an immediate view of what requires mapping, and what is complete
  • a much improved API for plugins, including better documentation, a reworked name proposal system, and an api/impl split for enigma code to ensure we can follow semver (I've discussed doing some plugin stuff with sciwhiz as well)
  • more convenience actions, like support for renaming an entire package in one go
  • tons of little improvements and bugfixes, like the better notification system, improved multiplayer support, support for searching methods, fields, and classes all at the same time, a pinned navigator to help you find obf things in a certain class, etc etc

screenshot (I tried to pack as many new features as I could in here):
Screenshot_20240325_120248

@ix0rai
Copy link
Contributor Author

ix0rai commented Mar 25, 2024

cc @marchermans

@marchermans
Copy link
Member

I didn't include a big description, because when I made this PR I thought the decision was already made based on my conversations with the maintainers, specifically @sciwhiz12. Either way, here are some key features for parchment:

* as mentioned, the added Vineflower decompiler produces what is IMO leagues better output than anything else

* our docker system allows you to reorganise and hide tabs such as the class tree, the implementations view, and others, to make more room for your code or display more information

* the statistic icons in the class tree provide an immediate view of what requires mapping, and what is complete

* a much improved API for plugins, including better documentation, a reworked name proposal system, and an api/impl split for enigma code to ensure we can follow semver (I've discussed doing some plugin stuff with sciwhiz as well)

* more convenience actions, like support for renaming an entire package in one go

* tons of little improvements and bugfixes, like the better notification system, improved multiplayer support, support for searching methods, fields, and classes all at the same time, a pinned navigator to help you find obf things in a certain class, etc etc

screenshot (I tried to pack as many new features as I could in here): Screenshot_20240325_120248

Okey but what are the downsides to using your version?

@ix0rai
Copy link
Contributor Author

ix0rai commented Mar 25, 2024

as far as I'm aware, there shouldn't be any. I watch the fabric repo and, with permission, pull in their changes when possible.

@ix0rai
Copy link
Contributor Author

ix0rai commented Mar 25, 2024

if you do find downsides (please, test this PR instead of discussing theory with me!) I'm happy to fix them for you. having input from the parchment team is important in order to make enigma better!

@ix0rai
Copy link
Contributor Author

ix0rai commented Apr 2, 2024

I'm working on Fabric's one, since Quilt's fork had horrible performance regressions in the past which made it unusable for my personal deobfuscation project. Afaik there has been work on resolving these, though I haven't conducted any new tests in the last few months

@NebelNidas missed the mention of performance regressions -- this was an issue in I think 1.6 when it released due to statistics being calculated on the main thread, it's since been fixed. I don't know of any other major regressions, I'd really appreciate some profiling data if you test and find that there are still issues!

@NebelNidas
Copy link

I just did a quick re-test, and while there may have been some improvements, your fork is still taking almost twenty times longer to index than Fabric's one (1min 10s for Fabric Enigma v2.4.2 vs. 21min 40s for Quilt Enigma v2.2.1). And after having waited for 22 minutes, all I got was a blank window (Swing probably crashed, idk).

this was an issue in I think 1.6

My report from last April mentions that 1.6 had performance regressions in the realm of around 100% longer indexing times, but the more problematic update was actually 1.7, which upped this number to at least 1000%. I have to say I'm not too keen on profiling this issue myself (given that I'm fairly invested in Fabric's fork), but if you wish to reproduce my results, this is the project I used. /gradlew enigma runs the Fabric version, ./gradlew enigmaQuilt the Quilt one.

@ix0rai
Copy link
Contributor Author

ix0rai commented Apr 3, 2024

interesting. I'll take a look

@ix0rai
Copy link
Contributor Author

ix0rai commented Apr 4, 2024

update: these performance regressions are purely issues with our swing gui (in fact, quilt enigma is a bit faster with indexing). fixes are being worked on!

@marchermans
Copy link
Member

Okey for now I will make this a draft.

@ix0rai
Copy link
Contributor Author

ix0rai commented Apr 4, 2024

why?

@marchermans marchermans marked this pull request as draft April 5, 2024 07:36
@sciwhiz12 sciwhiz12 added the enhancement Change to the code or other non-mappings files label Apr 10, 2024
@ix0rai ix0rai marked this pull request as ready for review May 1, 2024 01:36
@sciwhiz12 sciwhiz12 changed the base branch from versions/1.20.x to versions/1.21.x June 23, 2024 17:49
Copy link
Member

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

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

As I said on Discord, we will be trialling Quilt Enigma for the 1.21 release cycle. After 1.21, we'll discuss on what to do going forward.

@sciwhiz12 sciwhiz12 merged commit ff4c444 into ParchmentMC:versions/1.21.x Jun 23, 2024
2 checks passed
@ix0rai ix0rai deleted the quilt branch June 25, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Change to the code or other non-mappings files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants