What do you think of the use_item method in Scene_Battle?

[Tsukihime]

There's this method in Scene_Battle that is executed when a battler is going to use an item or skill.

def use_item  item = @subject.current_action.item  @log_window.display_use_item(@subject, item)  @subject.use_item(item)  refresh_status  targets = @subject.current_action.make_targets.compact  show_animation(targets, item.animation_id)  targets.each {|target| item.repeats.times { invoke_item(target, item) } }end

Any thoughts? Is there anything you would change to make it more RM flexible?

 

[TheoAllen]

Obviously, it executed some command when a battler use item. It determine how use item should be handled. Unfortunatelly, I can not think how to make it more flexible. In my battle system, I overwrite the method to show the action sequence. And Victor did that as well. Not sure for Symphony

 

[Shaz]

In what way is it INflexible? What would you need/want it to do that it doesn't do already, or does badly?


One recent request that I tracked back to this method is if you wanted to change the message that's shown in the log to include the name of the target ... the message is shown before the targets are acquired, so that would be impossible without changing it to acquire the targets right after getting the item.

 

[TheoAllen]

In what way is it INflexible? What would you need/want it to do that it doesn't do already, or does badly?

One recent request that I tracked back to this method is if you wanted to change the message that's shown in the log to include the name of the target ... the message is shown before the targets are acquired, so that would be impossible without changing it to acquire the targets right after getting the item.

It does NOT flexible. Yanfly script overwrite the method so badly and so does my script since they need to insert / change some stuff in the middle of the method. The way it was made, it's only usable for frontview battle system (show animation, and then call invoke item). You need to rework / overwrite the entire method in order to use any animated battler with action sequences.

 

[tyler.kendrick]

Well, one thing I've become very partial towards (because of how easily it allows for extensibility), would be to use blocks as callbacks.  However, just breaking up the methods so that they each handle a single responsibility would have been preferred.

Without callbacks, it would look like this:

def use_item item = subject_item targets = subject_use_item(subject, item) target_handle_item(targets, item)enddef subject_item return @subject.current_action.itemenddef subject_use_item(subject, item) @log_window.display_use_item(@subject, item) @subject.use_item(item) refresh_status return @subject.current_action.make_targets.compactenddef targets_handle_item(targets, item) show_animation(targets, item.animation_id) targets.each {|target| target_handle_item(target, item) }enddef target_handle_item(target, item) item.repeats.times { invoke_item(target, item) }endWith a callback and the method breakdown, it would look like this:

def use_item(&callback) item = subject_item targets = subject_use_item(subject, item) target_handle_item(targets, item) callback.call(item, subject, targets) unless callback.nil?enddef subject_item return @subject.current_action.itemenddef subject_use_item(subject, item, &callback) @log_window.display_use_item(@subject, item) return @subject.use_item(item) { |result| refresh_status results = @subject.current_action.make_targets.compact callback.call(item, subject, targets, result) unless callback.nil? results }enddef targets_handle_item(targets, item, &callback) show_animation(targets, item.animation_id) targets.each { |target| target_handle_item(target, item) } callback.call(targets, item) unless callback.nil?enddef target_handle_item(target, item, &callback) item.repeats.times { invoke_item(target, item) } callback.call(target, item) unless callback.nil?endThe advantage with the callback example is that in one-off use cases (perhaps when a subject/target/item is a specific id), you no longer need to override an entire method to do the check.  Simply adding a block to the method invocation will let you append new behaviors for that specific invocation.

This makes it even more extensible than simple method refactoring and makes it so that the only time you would actually override the method, is if you were truly changing the structure and formula involved in #use_item.

Now, for example, if you want to do something special when the subject id is the current player's actor id, you have some options...

If you want to completely redo the logic:

def subject_use_item(subject, item, &callback) return player_use_item(subject, item, &callback) if subject.id == $game_player.actor.id @log_window.display_use_item(@subject, item) @subject.use_item(item) { |result| refresh_status results = @subject.current_action.make_targets.compact callback.call(item, subject, targets, result) unless callback.nil? results }endIf you want to add new behavior:

def use_item(&callback)  item = subject_item  targets = subject_use_item(subject, item) { |item, subject, targets|    player_use_item(item, subject, targets) if subject.id == $game_player.actor.id  }  target_handle_item(targets, item)  callback.call(item, subject, targets) unless callback.nil?end

 

[Tsukihime]

I like the idea of using callbacks to further provide extensibility. I've used it in a couple odd places before, but never really found a good way to expose it to others. I've also seen a couple proof of concept scripts that really pushed the idea. Would be nice to see more work on integrating callbacks.

For maximum compatibility, single-responsibility is absolutely critical. This is because aliasing really only allows you to say this

def my_method # add some logic before it old_stuff # add some logic after itendFor me, separating these into their own method is an important step:

def targets_handle_item(targets, item)  show_animation(targets, item.animation_id)  targets.each {|target| target_handle_item(target, item) }end

This is because there's an inherent design decision that has to be made in your battle mechanics:1: do you show all animations, and apply the item one target at a time (what it does now), or

2: do you show one animation on one target and apply the item one target at a time

3: do you show all animations, apply all items on all targets, and then display the results of the action simultaneously

These are naturally contradicting cases and you can't have all of them at the same time: either you apply it to one target at a time, or you apply it to all targets at once.

The devs decided to go with #1, but I'm sure it's pretty common to want to use a skill that affects 5 targets and have all 5 targets collapse on death at the same time rather than collapsing one at a time.

However...callbacks might be able to address this problem...

Or just conditional branches with input specified by something like the item or skill that is going to be used.

 

[tyler.kendrick]

I completely agree and enjoy seeing a push for some semblance of SOLID principles in RGSS. 

The suggestion you mentioned is an entirely valid point, but because of the way I reconstructed the methods (with SOLID and callbacks in mind), you can simply override that method with the following code, without augmenting the behaviors or structure of the rest of use_item:

def targets_handle_item(targets, item) targets.each { |target| target_handle_item(target, item) { show_target_animation(target, item.animation_id) } }endI argue that this adheres to the single-responsibility use because "targets" in "targets_handle_item" implies behavior that applies to the collection of targets, rather than each individual - whereas the single-tense form of the method implies behavior that is encapsulated by, or acts on, individual targets.

Also, with callbacks (as noted immediately above), this structure enables a change in responsibilities without having to override the single form "target_handle_item".

...I love callbacks.

 

[Tsukihime]

def targets_handle_item(targets, item) targets.each { |target| target_handle_item(target, item) { show_target_animation(target, item.animation_id) } }end

RPG Maker supports two general types of animations

1. Screen

2. Target

One is meant for "all targets" while the other is meant for "one target".

The way it's written suggests that the animations act on each individual target

What would be a nice way to handle both scenarios?