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

bouncing indicator [bounty: 2 XTR] #2451

Closed
DreadKnight opened this issue Jul 28, 2023 · 11 comments
Closed

bouncing indicator [bounty: 2 XTR] #2451

DreadKnight opened this issue Jul 28, 2023 · 11 comments
Labels
bounty Some amount of our token, XatteR, is offered coding This issue requires some programming easy Good place to start if you're a new developer visuals Various things that easily catch the eye
Milestone

Comments

@DreadKnight
Copy link
Member

DreadKnight commented Jul 28, 2023

Hovering over active Dark Priest, pressing R and then materializing a unit somewhere will have the health/plasma indicator still bouncing.

@DreadKnight DreadKnight added coding This issue requires some programming visuals Various things that easily catch the eye easy Good place to start if you're a new developer bounty Some amount of our token, XatteR, is offered labels Jul 28, 2023
@DreadKnight DreadKnight added this to the 0.5 - Chimera milestone Jul 28, 2023
@andretchen0
Copy link
Contributor

andretchen0 commented Jul 28, 2023

I've mentioned it in passing before, so I hope this isn't unwelcome. Maybe it's worth broadening the discussion here.

It seems to me that these kinds of issues stem from the underlying programming paradigm used throughout the codebase:

startFn() {
    creature.updateValue(1);
}

stopFn() {
    creature.updateValue(0);
}

----

Creature.updateValue(value) {
   if (value === 0) ...
}

Note that creature has to get a method call for the changes. And if it hears the first updateValue from startFn, but something short-circuits stopFn, then creature will just keep chugging along until it's finally updated somewhere down the line. That's what's happening here:

  • The bounce is started in a queryHex callback in HexGrid, when a creature is hovered.
  • That relies on a separate HexGrid "leave" callback to stop the bounce.
  • But that callback doesn't get triggered if hotkeys are used in the meantime.

This is an inherently complicated pattern. "Teach a person to program, they'll create a bug. Teach a person to sync up state, and they'll have bugs for life."

In contrast, a more typical game loop might look like:

startFn() {
    gameState.value = 1;
}

stopFn() {
    gameState.value = 0;
}

----

Creature.update() {
    // run every tick
    if (gameState.value) ...
}

Or a reactive data store might look like:

store.startFn() {
    store.value = 1;
}

store.stopFn() {
    store.value = 0;
}

// Creature reacts automatically to store value changes it's interested in.

In the second and third examples, there's no need to explicitly tell the creature about the state change. In example 2, creature simply checks the value itself when it updates. In example 3, the creature update is triggered by a library when the state value updates.

But the current codebase is mostly like example 1.


It's far from a full solution for the codebase, but fwiw, on my local, I've been poking at HexGrid, abilities, and creatures, trying to uncouple them. But they're really tightly coupled. And there's a bunch of state syncing that needs to be kept in order in there. As an example, every time queryHex is called by an ability, the ability sends HexGrid some callbacks, which HexGrid transforms and then binds to every Hex.

I haven't finished unraveling everything going on in HexGrid, but I think so far that we could make it a lot less buggy if it mainly limited its activities to:

  • watching a handful of arrays of hex coordinates, e.g., game.interaction.highlightedHexes = [{x:0, y:2}, {x:1, y:2}]; and updating their display.
  • publishing events, e.g., mouse entered Hex 1, 9, mouse left Hex 1, 9.

If abilities need logic to distinguish between important/unimportant mouseovers and clicks – e.g., the mouse is in the allowed movement range of the active creature – then they should bring that, either themselves or by invoking a separate system.

Creatures could in turn watch something like game.interaction.activeHex = {x:0, y:2} – check whether or not they're on it. If they are, start bouncing health. If they're not, stop.


Anyway, just some ideas.

@andretchen0
Copy link
Contributor

andretchen0 commented Jul 28, 2023

So, with all of the above laid out, where do we go?

I've been working on making parts of the codebase less reliant on state – e.g. removing state from creature_queue and shifting from Hexs to pointFacade.

But I wonder if a bigger shift is in order.

Thoughts?

@DreadKnight
Copy link
Member Author

So, with all of the above laid out, where do we go?

I've been working on making parts of the codebase less reliant on state – e.g. removing state from creature_queue and shifting from Hexs to pointFacade.

But I wonder if a bigger shift is in order.

Thoughts?

Can have an easier fix at first with a "// TODO" around. I wouldn't mind a data store via Vue.js or so in the long run.

@andretchen0
Copy link
Contributor

andretchen0 commented Jul 28, 2023

Can have an easier fix at first with a "// TODO" around.

Sure thing. What do we do for a fix?

My preference for fixing this is not to fix it, but to remove the bounce. That way, at least it doesn't get into an inconsistent state. It can be put back in when the underlying system is less brittle.

Rationale

For fixes, I've mostly been going along with what's already in place, but fixes involving HexGrid are real bears. That system in particular is brittle and very tightly coupled; innocuous-seeming changes break things in unexpected ways. You fix a bug and two more pop up.

If we apply a fix over in hotkeys, we're just making it more tightly coupled to Creature.

Note that the Hex highlighting is also broken in the same way, but more subtly visually. Try:

  • At the start of a game, mouse over the active priest.
  • Press R.
  • Click the x to close the Creature Selection menu.
  • Mouse over the priest again – the hex doesn't react.
  • Mouse out then over the priest again – the hex reacts properly.

@DreadKnight
Copy link
Member Author

Can have an easier fix at first with a "// TODO" around.

Sure thing. What do we do for a fix?

My preference for fixing this is not to fix it, but to remove the bounce. That way, at least it doesn't get into an inconsistent state. It can be put back in when the underlying system is less brittle.

Rationale

For fixes, I've mostly been going along with what's already in place, but fixes involving HexGrid are real bears. That system in particular is brittle and very tightly coupled; innocuous-seeming changes break things in unexpected ways. You fix a bug and two more pop up.

If we apply a fix over in hotkeys, we're just making it more tightly coupled to Creature.

I get it, but would rather have the feature as it is with some inconsistencies here in there...

Note that the Hex highlighting is also broken in the same way, but more subtly visually. Try:

  • At the start of a game, mouse over the active priest.
  • Press R.
  • Click the x to close the Creature Selection menu.
  • Mouse over the priest again – the hex doesn't react.
  • Mouse out then over the priest again – the hex reacts properly.

Hmm, was expecting things like that to happen tbh, considering how things are currently coded. Feel free to open new issue for it.

@andretchen0
Copy link
Contributor

andretchen0 commented Jul 29, 2023

I get it, but would rather have the feature as it is with some inconsistencies here in there...

Ok. The on-the-ground problem here is that UI is changing and bits of the UI aren't getting notified of the change. For example:

  1. A Hex mouseover -> Starts creature's health bounce
  2. A key press -> Opens the dashboard
  3. A click -> Closes the dashboard
  4. 2 and 3 never communicated with HexGrid or the creature, so the creature is still "active" despite not being moused over.

Just code it

So a "just code it" solution could be:

  • The dashboard sends out a Phaser.Signal opening the dashboard.
  • HexGrid listens for the signal; when it hears it, it shuts everything off.

It's just a few lines of extra code. It's a little hacky though.

Down the line ... ?

A more comprehensive solution might be something like a stack of Screens. Where Screens are notified when they are moved in/out of the "active" position in the stack.

@DreadKnight
Copy link
Member Author

DreadKnight commented Jul 30, 2023

I get it, but would rather have the feature as it is with some inconsistencies here in there...

Ok. The on-the-ground problem here is that UI is changing and bits of the UI aren't getting notified of the change. For example:

  1. A Hex mouseover -> Starts creature's health bounce
  2. A key press -> Opens the dashboard
  3. A click -> Closes the dashboard
  4. 2 and 3 never communicated with HexGrid or the creature, so the creature is still "active" despite not being moused over.

Just code it

So a "just code it" solution could be:

  • The dashboard sends out a Phaser.Signal opening the dashboard.
  • HexGrid listens for the signal; when it hears it, it shuts everything off.

It's just a few lines of extra code. It's a little hacky though.

Down the line ... ?

A more comprehensive solution might be something like a stack of Screens. Where Screens are notified when they are moved in/out of the "active" position in the stack.

Sounds good. There's always as struggle between hacky fixes and proper fixes. A lot of the code will get revamped down the line and to be honest, players don't really care about the underlying coding, but about features and functionality (provided stuff is not buggy as hell).

@andretchen0
Copy link
Contributor

andretchen0 commented Jul 30, 2023

and to be honest, players don't really care about the underlying coding, but about features and functionality

The curve that technical debt follows means that hacky changes initially move fast. But as the technical debt builds up, things slow down because the codebase becomes fragile and hard to understand.

Past a certain threshold, it means more bugs, fewer bugfixes, and fewer features: the things that players care about.

@DreadKnight
Copy link
Member Author

and to be honest, players don't really care about the underlying coding, but about features and functionality

The curve that technical debt follows means that hacky changes initially move fast. But as the technical debt builds up, things slow down because the codebase becomes fragile and hard to understand.

Past a certain threshold, it means more bugs, fewer bugfixes, and fewer features: the things that players care about.

True, I'm aware. But we'll need to make sure to have a nice enough balance in the bigger image of things :)
Probably in the future will have a big (beta) release mostly focused on new features and one to follow after a while with bug fixes based on testing done. Current way is to cherry pick critical fixes from master onto the "stable" version, whenever possible.

@andretchen0
Copy link
Contributor

But we'll need to make sure to have a nice enough balance in the bigger image of things :)

For me anyway, the codebase is past the tipping point on tech debt.

It's hard to fix bugs. It's easy to make new ones.

That's not uncommon, especially for older JS projects. But I don't see how to move the project forward with new features before things get ironed out.

That's why I've been focused on refactoring.

@DreadKnight
Copy link
Member Author

But we'll need to make sure to have a nice enough balance in the bigger image of things :)

For me anyway, the codebase is past the tipping point on tech debt.

It's hard to fix bugs. It's easy to make new ones.

That's not uncommon, especially for older JS projects. But I don't see how to move the project forward with new features before things get ironed out.

That's why I've been focused on refactoring.

Yeah, I totally get it. Codebase already looking way better atm 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty Some amount of our token, XatteR, is offered coding This issue requires some programming easy Good place to start if you're a new developer visuals Various things that easily catch the eye
Projects
interface tweaks
Awaiting triage
Development

No branches or pull requests

2 participants