dont get update_menu_calling in Scene_Map

[Evgenij]

This is the method:

def update_call_menu if $game_system.menu_disabled || $game_map.interpreter.running? @menu_calling = false else @menu_calling ||= Input.trigger? call_menu if @menu_calling && !$game_player.moving? end endit looks complicated to me and I dont see why they need to use a @menu_calling variable for this.

Can someone explain it to me?

I mean you could do somethin like this:

def update_call_menu call_menu if can_call_menu? && Input.trigger? end def can_call_menu? return false if $game_system.menu_disabled return false if $game_map.interpreter.running? return false if $game_player.moving? return true endIs there a reason why it was done like it was done?

 

[Tsukihime]

Your proposed solution basically says

if menu can be opened  open menu

So what happens if the menu can't be opened when the menu button was pressed?

 

[Evgenij]

Nothing would happen or? I dont quite get where you are going

 

[Tsukihime]

Nothing would happen or? I dont quite get where you are going

Yes, nothing would happen, as you would expect.


This logic is correct: if the menu can't be opened, then it won't open if the player tried to open it.


Try replacing the default code with yours and see if there's any difference.


Testplay the game as a regular player. Move around the map and then open the menu.


Hint: UX

 

[Evgenij]

Ok will test the code, thanks for your reply.

But wait what is UX? 

*edit*

Ok I get it. If you hit the menu button it will cache the result till the player is not moving anymore.I didnt know that $game_player.moving? would return false when the player have reached a tile. I mean when I hit a direction button without pause and see my char moving I normally wouldnt expect that $game_player.moving? would return false every tile, it seems unlogical too me cause the player is still moving. 

I still dont know what you mean with UX, could you explain?

 

[Zeriab]

UX usually stands for User eXperience.

I totally agree that yours is better structured as it tells the story of the code much better. Be of course aware that setting the @menu_calling instance variable can have weird side-effects down the line.

As for why? Mistakes happens and usually the first iteration of our code is not the best. I bet there are loads of places in the standard script where you can refactor the code into a better state.

*hugs*

 -. Zeriab

 

[Engr. Adiktuzmiko]

I mean when I hit a direction button without pause and see my char moving I normally wouldnt expect that $game_player.moving? would return false every tile, it seems unlogical too me cause the player is still moving.

because the game actually only moves you 1 tile at a time... It only checks if you're pressing the move buttons once you finish the current movement (when you reach the next tile). After all, it takes time to move from 1 tile to the other so maybe they thought it is unnecessary to check for move command when you're still on a movement... And because of that, you basically finish moving after every tile that you moved.


Simpler answer:


because the moving? method doesn't check for the key. It only checks the position which returns true once you're on the next tile.


I find that a lot of the codes have been, let's just say, "simplified" to work with the intended purpose without excess processing. It quite decreases the flexibility of the original code though.

 

[Tsukihime]

UX usually stands for User eXperience.


I totally agree that yours is better structured as it tells the story of the code much better. Be of course aware that setting the @menu_calling instance variable can have weird side-effects down the line.


As for why? Mistakes happens and usually the first iteration of our code is not the best. I bet there are loads of places in the standard script where you can refactor the code into a better state.


*hugs*


 -. Zeriab

Do you mean that the instance variable is non-optimal or even unnecessary?

because the game actually only moves you 1 tile at a time... It only checks if you're pressing the move buttons once you finish the current movement (when you reach the next tile). After all, it takes time to move from 1 tile to the other so maybe they thought it is unnecessary to check for move command when you're still on a movement... And because of that, you basically finish moving after every tile that you moved.

What would happen if they checked for movement while you were in the middle of movement?

 

[Zeriab]

Look at the code. Is @menu_calling used in any other methods? Can you tell me that from the given code alone?

No. It might not be used elsewhere, its use might work regardless of whatever we set here, or if we are most unlucky some other methods depends on the @menu_calling being set correctly.

Now, let's assume we can safely replace @menu_calling with menu_calling. Look, the worry has gone. We can feel much more confident about the refactoring.

  def update_call_menu    if $game_system.menu_disabled || $game_map.interpreter.running?      menu_calling = false    else      menu_calling ||= Input.trigger?      call_menu if menu_calling && !$game_player.moving?    end  endDoing this we quickly notice the odd menu_calling = false line. Quite a strong indicator that the instance variable is used somewhere else.

But let's assume that it is indeed not called anywhere else. Then the code with the local variable is simply just better.

@Evgenij: Is @menu_calling used anywhere else? And if so, can you check why it does not include the !$game_player.moving? condition?

 

[Tsukihime]

Look at the code. Is @menu_calling used in any other methods? Can you tell me that from the given code alone?


No. It might not be used elsewhere, its use might work regardless of whatever we set here, or if we are most unlucky some other methods depends on the @menu_calling being set correctly.


Now, let's assume we can safely replace @menu_calling with menu_calling. Look, the worry has gone. We can feel much more confident about the refactoring.


Doing this we quickly notice the odd menu_calling = false line. Quite a strong indicator that the instance variable is used somewhere else.

Yes, I can understand what you mean by the extra concerns there may be if we are relying on an instance variable that someone else might (unintentionally) use without being aware of its use in other places.


By using a local variable, others can introduce as many instance variables as they want, but it wouldn't affect my method.

But let's assume that it is indeed not called anywhere else. Then the code with the local variable is simply just better.

You're right, if that assumption were true, then a local variable is better.


But I think the instance variable is used somewhere else, which is why I don't think a local variable is an appropriate refactor for this method.

 

[Zeriab]

There's more than that. Why can @menu_calling be set to true in cases where call_menu is actually not called?

Which leads to what possible could happen if the method is called multiple times. When the first if condition is false @menu_calling can only change state from false to true, not from true to false.

Does the line read we have to wait possible multiple executions of the method from the B button is triggered until the call_menu method is executed?

So are my following comments correct?

  def update_call_menu    if $game_system.menu_disabled || $game_map.interpreter.running?      # Reset menu call      @menu_calling = false    else      @menu_calling ||= Input.trigger?      # Wait until the player has stopped moving before calling the menu      call_menu if @menu_calling && !$game_player.moving?    end  end
This could explain the reasoning. I have not looked at the rest of the code, so I don't know. But even if this is correct I would still not be satisfied. I can use method names to tell the story. To teach what goes on.

  def update_call_menu    if menu_disabled?      clear_menu_call    else      trigger_menu_call if Input.trigger?      # The menu call may happen multiple frames after the trigger_menu_call      if menu_call_pending? && can_call_menu?        call_menu      end    end  end
A vast improvement over what we started with. The specific conditions are abstracted away and the important temporal logic is highlighted.

*hugs*

 - Zeriab

 

[Tsukihime]

def update_call_menu if menu_disabled? clear_menu_call else trigger_menu_call if Input.trigger? # The menu call may happen multiple frames after the trigger_menu_call if menu_call_pending? && can_call_menu? call_menu end end end

This would be a more accurate re-factor, and may be more obvious why we even have a @menu_calling instance variable in the first place

However, I'm sure there would still be questions asking "why are we using this instance variable at all?"

Why can @menu_calling be set to true in cases where call_menu is actually not called?

This is what I think is the key question, though I left it out and didn't do a really good job hinting at it.

In a code review, when I see that, I'd be wondering

1. Is this correct behavior?

2. Or is this just a bug that someone made because they wrote the code once, seems to work based on limited testing, and then never bothered to refactor it?

3. Perhaps it's just because they wanted to simplify it to achieve the intended purpose, as Estriole suggests?

4. But then what is this "intended purpose"?

5. Maybe the dev was coding under influence?

I'm led to believe that it's somewhere between "wrote once, seems to work, let's move on" and "simplify it"