Skip to content

Conversation

@Cokemonkey11
Copy link
Contributor

No description provided.

@Cokemonkey11
Copy link
Contributor Author

@Frotty can I trouble you for a review? I reworked this to use wc3libs

It seems to work for me!

image

@Frotty
Copy link
Member

Frotty commented May 11, 2020

Seems fine, but will bloat the setup some more.
Also this will not fix runmap for existing projects, of course.

@Cokemonkey11
Copy link
Contributor Author

Hey @Frotty

Thanks for the comment. My build looks like 19MB compared to 16MB of the latest release.

Assuming that there is no additional bloat as part of releasing (proguard or something), is this acceptable?

If not, what level would I need to aim for to get this to a more acceptable level?

Thanks!

buildproject/
compiled.j.txt
myname/
ptrtestproject/
Copy link
Member

Choose a reason for hiding this comment

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

don't add your local test folders to gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these are all generated by the project itself - what do you mean "local test folders"?

Copy link
Member

Choose a reason for hiding this comment

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

what makes u think

compiled.j.txt
myname/
ptrtestproject/

would be files generated by this project?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would be files generated by this project?!

The project has tests, right? What kind of tests does it have? Wurst tests

In fact I know these are generated by some gradle task, because otherwise I wouldn't have these files to ignore

@Frotty
Copy link
Member

Frotty commented May 22, 2020

Assuming that there is no additional bloat as part of releasing (proguard or something),

?! Proguard shrinks the .jar

If not, what level would I need to aim for to get this to a more acceptable level?

The main question is probably whether this game path option in the setup, or it's auto-detection, is useful at all.
If the path is not set, or is invalid, the language server will use the same wc3libs-detection to detect the game path, that you now added here, anyway.
Hence the option is an optional override, e.g. if the auto-detection does not work or you have multiple installations.
Setting it to the auto-detected value is pretty superfluous imho and is why I wondered about u doing it in the first place.

@Cokemonkey11
Copy link
Contributor Author

@Frotty do you think I should instead just remove it?

That feels like a separate issue from merging this, since all I've done is improve an existing feature - at least in my view

@Cokemonkey11
Copy link
Contributor Author

@Frotty let's make a decision here

If we don't want this feature then I can instead remove the path setup.

But maybe if we want to remove the path setup, we should merge wurstscript/WurstScript#951 first

@Cokemonkey11
Copy link
Contributor Author

@Frotty now that wurstscript compiler is updated, do you have a stronger opinion on this one? I can remove path-setup in a new PR if you want

@Frotty
Copy link
Member

Frotty commented Nov 19, 2020

I think the main issue right now is that the path is reset to "null" on grill install when a custom one is set.
This is fine if wc3 is at default location, but not otherwise.

@Cokemonkey11
Copy link
Contributor Author

@Frotty that's an unrelated problem, right? Let's address that in a different PR.

Or are you saying that this PR has a regression?

@Frotty Frotty closed this Dec 1, 2020
@Cokemonkey11
Copy link
Contributor Author

@Frotty as discussed, we should instead remove the Game Path feature from Wurst Setup GUI

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