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

added colors #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

added colors #1

wants to merge 3 commits into from

Conversation

TvL2386
Copy link

@TvL2386 TvL2386 commented Sep 10, 2020

No description provided.

TvL2386 and others added 3 commits September 10, 2020 19:42
@MarsEdge
Copy link
Owner

I'm quite elated that someone is actually using my code. I haven't seen much use and this is a welcome surprise. Sadly, I'm mostly retired from GW2 such that I cannot test this. Since I can't test it.... I can't confidently merge it. My website is now down, but my post about stopping development can be found here

That being said, a couple things stand out.

The profession colors appear hardcoded. arcdps has an api for getting the colors used in the arcdps UI settings. I'd encourage using that over separate hardcoded values.

It looks like you're using a newer version of Visual Studio. That not a big deal, but it should probably not be in this PR.

Changing how the hash is made(and/or location of GW2-64.exe) also makes sense for you, but not for this PR.

You probably don't want to do custom patches to imgui itself. I use a very specific version that is also used by arcdps. You'd want to make a separate function that does what you want. I'm actually somewhat surprised this works at all given that the arcdps imgui context is the one that ends up running everything.

The profession colors don't appear to be optional, yet you removed a TODO saying 'add optional profession colors'

I don't really know what to say. I don't feel comfortable merging it(even if/when the above are fixed), but if you want to fork the project and start doing things yourself.... go for it. I'm happy to answer questions about the code. You can also find me/other knowledgeable people in the Elite Insights discord

I'll leave the PR open so others will see it.

@TvL2386
Copy link
Author

TvL2386 commented Sep 11, 2020

Hi MarsEdge,

It seems I accidentally created a pull request when I just wanted to merge it to my master :-)
Sorry for that. I totally agree with all the things you said. This request has much more than would be appropriate to merge and will disrupt your development environment.

I heard that you had retired developing this tool and received a request to "add colors like arcdps". So this is actually my first swing at it (new to C++ and Visual Studio).

I indeed want to use the colors that can be manually set in arcdps, however I didn't see how to get those values from arcdps internally. Feel free to close the pull request when you see fit.

Thanks for the feedback and this awesome tool! :-)

Meme-sys pushed a commit to Meme-sys/GW2-ArcDPS-Boon-Table that referenced this pull request Feb 24, 2021
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