Maruno17 / pokemon-essentials

A heavily modified RPG Maker XP game project that makes the game play like a Pokémon game. Not a full project in itself; this repo is to be added into an existing RMXP game project.
Other
208 stars 398 forks source link

Minor new additions for v20 #147

Closed Golisopod-User closed 2 years ago

Golisopod-User commented 2 years ago

A number of small additions for v20:

Maruno17 commented 2 years ago

I don't want to allow Pokémon to naturally forget HM moves for the simple reason that you could be in the middle of using Surf/Strength/Dive/etc. at the time. It wouldn't make sense for you to continue to use those effects if your Pokémon no longer know the move. If these are not field moves in a user's game, they can delete the HM items that teach them.

It's unnecessary to create a new property in trainers.txt for the name of their Mega Ring. The better solution is to simply define all the Mega Rings in items.txt, and make the code require that an NPC trainer have one in their inventory in order to Mega Evolve (rather than assume they have one).

I noticed you trying to sneak in a check of the mtime of the Plugins folder. That suggestion was rejected. Remove it.

There is no need to check whether an item is a TR in def consumed_after_use?. @consumable is set when the item is registered, and that checks what kind of item it is.

Why have you made the ItemHandlers::UseFromBag proc for machines next -1? 0 is the value for when the item isn't used after all. If you were worried about the "Can't use that here." message, note that the code just above it uses if intret >= 0 which includes 0 and will return from the method without showing that message anyway. The "Can't use that here." message looks like it's entirely unused, actually, due to what ItemHandlers.triggerUseFromBag returns.

The two ItemHandlers::UseFromBag procs can be merged, since the only difference between them is the last argument for pbMoveTutorChoose. The code in def pbUseItem can worry about whether to delete the item after use, so the procs don't have to.

You also didn't mention in your list above that you've added a quantity window to the Mart screen, to always show how many of the selected item you already have. I have nothing to say about it; just mentioning it for the sake of listing everything.

You can remove the change to trainers.txt.

Golisopod-User commented 2 years ago

I've made the necessary changes for the TM Item Handlers and disabled the setting to allow forgetting HM Moves by default.

The Mega Item name was a hardcoded string inside the function pbGetMegaRingName, which is completely unrelated to any . All I've done is move that hardcoded string and made it a PBS property instead, to make it easier to edit. I don't see why that prompts the need to change entire Mega Ring system for NPC Trainers when it works fine as is.

Unless, what you're trying to suggest is that we remove the MEGA_RINGS array from the settings, and instead create a new flag called "KeyStone" or "MegaRing" to denote the Mega Evolution items and check for both the player's and NPC's Mega Rings there, which could work. But that'd also involve changing pbHasMegaRing? to account for this.

The item quantity window in the Mart scene was a last minute addition and I forgot to edit my original PR comment to include that. That's on me.

The Plugin folder check is pretty much harmless and all it does is make the Plugins system more intuitive and easier to use. I fail to see why it should be removed.

Maruno17 commented 2 years ago

Since you're requiring the dev to write something extra into trainers.txt already (the name of the Mega Ring item they have), it's just as easy to get them to add that Mega Ring item to the trainer's inventory instead. This also makes Mega Rings consistent between the player and NPCs, in that both would need the actual item in order to Mega Evolve (rather than inherently allowing NPCs to do so).

The code might as well be changed in the way you described (having a "MegaRing" flag for items, checking an NPC's inventory for an item with that flag to allow them to Mega Evolve). Hardcoding the names of NPC Mega Rings was never a great idea, and your current code change doesn't fix that, just shifts it into trainers.txt (and invents a whole new messages section just for a few item names, when there's an existing messages section containing all item names). The one downside here is that partner trainers don't currently have an inventory of items, so they wouldn't be able to Mega Evolve until that was fixed.

We've talked about checking the Plugin folder's mtime already, and it was rejected.

Golisopod-User commented 2 years ago

I've removed the Trainer Mega Item property and made the trainer's Mega Evolution work based whether the trainer has an item with the flag "Mega Ring" in the items array. I've also added an array of items for the ally in the battle, which is used for the partner's mega evolution check for now, but could be used for more AI related things in the future.

I've removed the Plugin Manager compilation change, but I'm gonna request you to reconsider it once more, because its a harmless addition and all it does is make the PluginManager more easy-to-use.

FL- commented 2 years ago

I also request you to reconsider Plugin Manager compilation change.