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

[1.18.x] Remove ImGui in favor of online web service #55

Closed
wants to merge 10 commits into from
Closed

[1.18.x] Remove ImGui in favor of online web service #55

wants to merge 10 commits into from

Conversation

andriihorpenko
Copy link
Contributor

@andriihorpenko andriihorpenko commented May 26, 2023

Introduction

This PR merges original 1.18-online branch with additional commits from 1.19 branch.

Effects

Notes

  • I have not included any changes to the GitHub workflows to not ruin current setup for newer versions of the game itself, so this is purely up to the maintainer ;)
  • I have completely revamped ru localization as it was clearly generated by some sort of Google Translator. Not cool, not cool! If this gets through, ru localization can be ported to 1.19 branch.

@tasgon
Copy link
Owner

tasgon commented May 28, 2023

Thank you so much for the contribution! I haven't had the time yet to do a review yet, but I'll do one today.

@tasgon
Copy link
Owner

tasgon commented May 30, 2023

Looking through it, I have a few thoughts:

  1. I probably would want to include the new workflows (and modify them for 1.18), since they vastly simplified my life when it comes to uploading mods to CF/Modrinth.
  2. I'm doing a diff between 1.19 and your branch and I noticed a lot of formatting line diffs (i.e. a lot of lines where the only difference is a comma at the end). This isn't that bad of a problem, but it looks like a few formatting commits weren't picked up.
  3. Fixing [1.18.2] Unable to see mob TPS, some incompatibility #52 seemed like a great idea before I found out about Does Potato Tick, which also implements a fix of its own by incorporating Observable's tracking in its own mixin. If we fix the problem on our end but DPT doesn't (or people use an outdated version of DPT), this could potentially mean entities get double-counted.
  4. I really appreciate the Russian translation changes, but I would rather they be in a separate PR. Not a showstopper, but I might actually just pull the changes out and put them on the 1.19 branch before merging this PR so both versions benefit from the translation.

Overall, I like most of the PR, but there's a few changes I want to make on the 1.19 branch before backporting, and quite honestly I'm not sure what to do regarding point 3.

@andriihorpenko
Copy link
Contributor Author

  1. Good point, I can include workflow changes in this PR if that’s okay.
  2. I’ll revisit formatting commits to include those changes in 1.18 branch as well.
  3. I believe we can specify a strict version range of DPT in mods.toml. Another option is to add a notice on CF/Modrinth about such side effect starting from 2.3.0. Lastly, we might want to contact DPT devs to resolve this issue.
  4. I’ve included a few new translation lines regarding current profiling status, that’s why I did not extract those changes into a separate PR. I’ll gladly make a next PR targeting 1.19 branch with those changes.

@andriihorpenko andriihorpenko deleted the feature/imgui_to_online branch July 11, 2023 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants