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

Godlet Dashboard Toggle #1237

Merged
merged 5 commits into from
Oct 12, 2017
Merged

Godlet Dashboard Toggle #1237

merged 5 commits into from
Oct 12, 2017

Conversation

sammorrow
Copy link
Contributor

Referencing #1147

Adds new function to the UI class, godletToggle. godletToggle behaves similarly to toggleDash but calls showCreature with a random, available, affordable creature.

The Dark Priest's Godlet Scanner now calls godletToggle instead of toggleDash.

Copy link
Member

@DreadKnight DreadKnight left a comment

Choose a reason for hiding this comment

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

Very nice to see a PR. Will comment more after testing.

const typeMap = activePlayer.availableCreatures.filter(el => el.length);
const deadOrSummonedTypes = activePlayer.creatures.map(creature => creature.type)
const availableTypes = typeMap.filter(el => !deadOrSummonedTypes.includes(el))
// optional: randomize array, grab a new creature every toggle
Copy link
Member

Choose a reason for hiding this comment

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

Capitalized letters for comments, for style consistency.

availableTypes[i] = availableTypes[j];
availableTypes[j] = temp;
}
// grab the first creature we can afford (if none, default to priest)
Copy link
Member

Choose a reason for hiding this comment

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

Capitalized...


godletToggle: function() {
Copy link
Member

@DreadKnight DreadKnight Oct 12, 2017

Choose a reason for hiding this comment

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

toggleGodlet would probably be a better function name, hmm...

I haven't tested the patch yet, but as far as I can tell, this might have been approached better as simply having a random bolean parameter for toggleDash function, because there are quite a few other UI fixes/patches that rely on toggleDash function itself. Gonna comment on the PR after testing anyway.

@DreadKnight
Copy link
Member

DreadKnight commented Oct 12, 2017

I've tested this, as as I was saying, because of the function name change for the Godlet Printer ability, the game ends up being rather quirky, best approach would be to keep old function name and have a boolean parameter for random unit selection in order to avoid having to figure out all the issues we dealt with before.

@sammorrow
Copy link
Contributor Author

Aw dangit, didn't want to bloat the other function but neglected performance quirks-- refactored.

@DreadKnight
Copy link
Member

@sammorrow it still introduces that bug when activating the ability and then canceling it, if you notice...

@sammorrow
Copy link
Contributor Author

Altered a condition in closeDash, querymove is now triggered on dash close when activeCreature, not selectedCreature, is a Dark Priest.

@DreadKnight DreadKnight merged commit 7bacf92 into FreezingMoon:master Oct 12, 2017
@DreadKnight
Copy link
Member

@sammorrow This also broke the recent fix for #1208 for some reason...

@DreadKnight
Copy link
Member

DreadKnight commented Oct 12, 2017

Also, selecting a unit for materialization doesn't indicate that Godlet Printer ability is actually selected...
It should have a colored frame around it and not blink.

@DreadKnight
Copy link
Member

Also, if materializing a unit within active Dark Priest's movement range, the range doesn't get updated.
Showing up as Dark Priest could walk over that unit, unless he moves to an empty spot first to fix that.

@DreadKnight
Copy link
Member

DreadKnight commented Oct 12, 2017

Regarding my last comment, seems that issue was around but only been made visible because of another PR we're experimenting with atm, so I'm going to open up some new issues related to that as required :)

@DreadKnight
Copy link
Member

I've reverted this for now, feel free to open a new PR when it's a bit less problematic and I'll test it again.

@sammorrow
Copy link
Contributor Author

@DreadKnight Ah, sorry bout the trouble.

I've reverted back to the most recent master myself to get a clean PR going, but I'm running into some odd behavior that differs from the live site. With the current master code, if I open the scanner menu, select a creature, then close the menu, I run into the 'uncancelled closeDash' bug (you have to type R to break out of your action and R again to re-open the menu). I checked the code on the live site--closeDash is slightly different there. It's being fed a boolean ('materialize'), it doesn't run into that bug, and it doesn't generate extra 'cancelled' messages when you click the 'materialize' button. Where'd that version of closeDash go?

@DreadKnight
Copy link
Member

DreadKnight commented Oct 13, 2017

@sammorrow No worries. The live version is considered as stable and it's not updated constantly, but only when new version is released or when certain hotfixes are in order (not quite the case atm anyway).

The master here is basically 0.4 unstable, so it's a development version and that patch was added in it.

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