Improving player movability condition

[Tsukihime]

How can this method be improved? It's clearly checking too many different conditions.

class Game_Player  def movable?    return false if moving?    return false if @move_route_forcing || @followers.gathering?    return false if @vehicle_getting_on || @vehicle_getting_off    return false if $game_message.busy? || $game_message.visible    return false if vehicle && !vehicle.movable?    return true  end end

 

[??????]

It's clearly checking too many different conditions.

Its only checking too much if certain steps are not required. Otherwise its checking exactly what it needs to in order to perform the desired function.

Not checked, but i would think that the vehicle method returns data for the vehicle object, which will likely have logic incorporated for checking if getting on and off, so you could perhaps ditch the 'return false if @vehicle_getting_on || @vehicle_getting_off', but who knows ^_^

 

[Tsukihime]

How is checking whether the message box is showing or not related to checking whether you're getting in a vehicle?

 

[??????]

lol, they are completely unrelated - naturally, however, in this situation they are both restrictions the engine dev's felt where necessary. Personally I wouldn't mind being able to move while im supposed to be reading an important cutscene message

 

[Tsukihime]

however, in this situation they are both restrictions the engine dev's felt where necessary.

Which is OK, but requiring a set of conditions to be met in order to proceed with an action doesn't mean you have to write them in one method.

 

[??????]

oh for sure..

According to my own benchmarking tests though, more methods = slower code (in most cases). So for example, if you separated each of those checks into its own method, like this...

class Game_Player  def movable?    any_moving?  end  def any_moving?    moving? || movable_check2? || movable_check3? || movable_check4? || movable_check5?  end  def movable_check2?    @move_route_forcing || @followers.gathering?  end  def movable_check3?    @vehicle_getting_on || @vehicle_getting_off  end  def movable_check4?    $game_message.busy? || $game_message.visible  end  def movable_check5?    vehicle && !vehicle.movable?  endend Could potentially result in code that runs slower

Also - checking a variable value within a method is always faster than calling a method that returns the same value.  Its one of the many 'perks' \s of the ruby code interpreter

Edit:

That being said, perhaps 'speed' isnt the priority, perhaps its flexibility. In which case, having the separated methods would be much more advantageous.

 

[Tsukihime]

I would trade a few milliseconds* of processing for infinitely more flexible code to work with.


*untested, just a random figure

 

[Lemur]

How can this method be improved? It's clearly checking too many different conditions.

class Game_Player def movable? return false if moving? return false if @move_route_forcing || @followers.gathering? return false if @vehicle_getting_on || @vehicle_getting_off return false if $game_message.busy? || $game_message.visible return false if vehicle && !vehicle.movable? return true end end

[  moving?,  @move_route_forcing,  @followers.gathering?,  @vehicle_getting_on,  $game_message.busy?,  $game_message.visible,  vehicle && !vehicle.moveable?].none?

Though I would add a concept of movable to each of the objects, that way you can just ask each one of them in turn depending on what objects your character is tied to.

 

[♥SOURCE♥]

[  moving?,  @move_route_forcing,  @followers.gathering?,  @vehicle_getting_on,  $game_message.busy?,  $game_message.visible,  vehicle && !vehicle.moveable?].none?Though I would add a concept of movable to each of the objects, that way you can just ask each one of them in turn depending on what objects your character is tied to.

Why make the method about three times slower and less readable?

 

[Lemur]

Because you can do something like this later:

player.moveables = []player.moveables << (whatever)# Ex objects as hashes:# {name: 'ship', active: false, moveable?: (single method to determine moveability)} player.moveables.select(&:active).none?(&:moveable?)Tell me honestly, is speed that large of a concern to you? You're wrong, but is it really?

foo = -> { puts 'foo' }bar = -> { puts 'bar' }baz = -> { puts 'baz' } Benchmark.measure { 100000.times { [foo,bar,baz].none? { |proc| proc.call() } } }.real# => 1.002677 Benchmark.measure { 100000.times { foo.call() || bar.call() || baz.call() ? false : true } }.real# => 0.917967Please, if you're going to try and make a point on speed, post the benchmarks instead of spreading blatantly false information.

 

[♥SOURCE♥]

Because you can do something like this later:

player.moveables = []player.moveables << (whatever)# Ex objects as hashes:# {name: 'ship', active: false, moveable?: (single method to determine moveability)} player.moveables.select(&:active).none?(&:moveable?)

Nice, but even less readable for non programmers. And most RPG Maker users are not programmers and don't have interest in becoming one. As I mentioned in the other thread, people that may change one or two things in the scripts to fix or adjust something for their games, would appreciate readability and "captain obvious style" code commenting.

Tell me honestly, is speed that large of a concern to you? You're wrong, but is it really?

foo = -> { puts 'foo' }bar = -> { puts 'bar' }baz = -> { puts 'baz' } Benchmark.measure { 100000.times { [foo,bar,baz].none? { |proc| proc.call() } } }.real# => 1.002677 Benchmark.measure { 100000.times { foo.call() || bar.call() || baz.call() ? false : true } }.real# => 0.917967Please, if you're going to try and make a point on speed, post the benchmarks instead of spreading blatantly false information.

I was wrong in the speed difference, yes. Still, it is slower and less readable.

Spoiler

class Game_Player < Game_Character def movable2? [moving?, @move_route_forcing, @followers.gathering?, @vehicle_getting_on, $game_message.busy?, $game_message.visible, vehicle && !vehicle.moveable?].none? endend Script Call Event Command:

eval %Q(require 'benchmark/ips'Benchmark.ips do |x| x.configtime => 5, :warmup => 2) x.report("rtp movable") { $game_player.movable? } x.report("lemur movable") { $game_player.movable2? } x.compare!end) 

Speed is a concern for every RPG Maker user, the RGSS Player is quite slow for what it does.

 

[Lemur]

If I'm being honest, the entirety of RGSS 3 needs to have a lot of cleanup to it. It's too dependent on procedural code practices and tends to leave a lot of mess around. That's common for people just starting Ruby, especially coming from C type backgrounds.

What I'm suggesting is using a far more OO approach of Tell, Don't Ask. It does a huge amount for cleaning up code, and is a very good rule to live by for object systems much like this. FP and OO are good paradigms to mix for this type of thing, Procedural ends up in a mess for data driven objects like this.

 

[♥SOURCE♥]

If I'm being honest, the entirety of RGSS 3 needs to have a lot of cleanup to it. It's too dependent on procedural code practices and tends to leave a lot of mess around. That's common for people just starting Ruby, especially coming from C type backgrounds.

Yes, absolutely. GDI+ for drawing on top of that.

What I'm suggesting is using a far more OO approach of Tell, Don't Ask. It does a huge amount for cleaning up code, and is a very good rule to live by for object systems much like this. FP and OO are good paradigms to mix for this type of thing, Procedural ends up in a mess for data driven objects like this.

I agree with that, I hope the next iteration of the program improves the Player and the standard library.

 

[Lemur]

http://forums.rpgmakerweb.com/index.php?/topic/34937-rgss-improvements/ - Forked that part of the conversation out. I'd be more than willing to help with it, but as it is now it's just too restrictive.