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

dependencies: back to giu upstream #335

Open
gucio321 opened this issue Jul 31, 2021 · 7 comments · May be fixed by #336
Open

dependencies: back to giu upstream #335

gucio321 opened this issue Jul 31, 2021 · 7 comments · May be fixed by #336

Comments

@gucio321
Copy link
Contributor

Is your feature request related to a problem? Please describe.
At the moment we're using a fork of giu (ianlin/giu) but structure and dependencies of the fork was changed and now updating it and aplying a new features is a bit painful

Describe the solution you'd like
we can easy switch to upstream of giu (AllenDang/giu)

Additional context
a new solution added by giu authors looks very cool and we may really want to introduce them.

@gravestench
Copy link
Contributor

gravestench commented Aug 3, 2021

@ianling Didn't you do a giant refactor on your fork?

EDIT: after discussion, yes.

giu originally had an entire copy of imgui-go inside of the repo.

@ianling refactored giu to use a fork of imgui-go, with the edits that @AllenDang had made. Then, this fork was sent to @AllenDang. There's currently a 60+ commit delta between Allen's imgui-go fork and upstream.

This is just my opinion, but I think that the real issue here is the fact that @AllenDang is not using upstream imgui-go. He seems to fancy making changes in his own fork as opposed to making PR's and getting those features added to upstream imgui-go.

@AllenDang
Copy link

AllenDang commented Aug 4, 2021

@gravestench The reason why I'm using my own fork of imgui-go, is I intend to make giu a complete Desktop UI framework (AFAIK this is not the purpose of imgui itself), but upstream imgui-go's purpose is to create a plain imgui wrapper for go.

Refer this inkyblackness/imgui-go#153.

So I will merge power saving patch to chill CPU and GPU usage, I emebed ImPlot and ImNode and more extentions in future into imgui-go (those extensions have to be placed within imgui.h to compile).

Why this will become an issue whether giu use upstream imgui-go or not?

@gravestench
Copy link
Contributor

Why this will become an issue whether giu use upstream imgui-go or not?

It's an issue when we encounter a bug in your fork of imgui-go, which is fixed in upstream imgui-go, and we want the fix. That refactor Ian had done was all about reducing the pain of syncing your imgui-go fork with upstream.

@AllenDang
Copy link

@gravestench What kind of bug in my fork of imgui-go?

@gravestench
Copy link
Contributor

One such issue was a hard limit on number of columns that could be displayed in a table; we were trying to display +250 columns as one of the CSV's in the D2 files actually has that many columns in it!

@AllenDang
Copy link

AllenDang commented Aug 5, 2021

@gravestench Did anyone rise this issue to giu? Did I reject to help?

I just feel uncomfortable about the word you are using like He seems to fancy making changes in his own fork as opposed to making PR's and getting those features added to upstream imgui-go.. What does fancy making changes indicate?

And are you doing the same thing here? Make fancy changes to own fork of giu as opposed to making PR's and getting those features added to upstream giu?

But, fine, I can sense that you are some kind of opposite, do what you think is right and have fun.

@gravestench
Copy link
Contributor

What does fancy making changes indicate?

My apologies if that came off a bit harsh.

I'm just saying it's less effort to make changes to your fork of imgui-go and call it a day instead of making PR's to upstream. We're all doing it, and I'd like for this to change, but not sure what it means for your development, or if it's even possible given the cgo limitation. It may require another approach like making a new imgui-for-giu repo, which acts as a wrapper for imgui-go and the various imgui-go extensions you want to use inside of giu.

And are you doing the same thing here? Make fancy changes to own fork of giu as opposed to making PR's and getting those features added to upstream giu?

Back in April, @ianling did a huge refactor of your codebase, moving the static copy of imgui-go out of giu and into its own fork. Then, he pulled upstream imgui-go into this fork. Then, he applied all of your alterations. Altogether, this was something like +14K/-6K, and well over a week of effort on his part. He went to a lot of trouble testing everything to make sure it all worked well and the same, as well as integrating your changes in such a way that merging upstream had minimal conflicts. Then, he transferred ownership of that fork to you.

I just checked out that commit (ea60551) and did git pull upstream main and the merge was fairly painless (it took only me a few minutes). However, if I check out HEAD on your imgui-go fork and try to merge upstream, there are 20 pages of merge conflicts.

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 a pull request may close this issue.

3 participants