tasgon / observable

See what's lagging your server. 15M+ downloads on CurseForge
https://www.curseforge.com/minecraft/mc-mods/observable
Mozilla Public License 2.0
79 stars 8 forks source link

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

Closed andriihorpenko closed 1 year ago

andriihorpenko commented 1 year ago

Introduction

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

Effects

Notes

tasgon commented 1 year ago

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 commented 1 year ago

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 #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 commented 1 year ago
  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.