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

Misc fixes #1397

Merged
merged 14 commits into from Sep 30, 2018

Conversation

Projects
None yet
4 participants
@Albeleon
Member

Albeleon commented Jul 8, 2018

No description provided.

@@ -423,6 +423,13 @@ class Game_Actor : public Game_Battler {
*/
const std::vector<int16_t>& GetWholeEquipment() const;
/**
* Checks whether it has an item equipped.

This comment has been minimized.

@carstene1ns

carstene1ns Jul 9, 2018

Member

Suggestion: Checks if the actor has a specific item equipped.
Also, below: Whether the item is equipped

comment indentation is also wrong (as always).

@@ -453,3 +453,19 @@ void Game_System::OnSeReady(FileRequestResult* result, int volume, int tempo, bo
Audio().SE_Play(path, volume, tempo);
}
bool Game_System::IsSoundStopFilename(RPG::Sound sound) {

This comment has been minimized.

@carstene1ns

carstene1ns Jul 9, 2018

Member

This is IMO not really needed, as the file list is cached on startup.

Update();
refresh = false;
Refresh();

This comment has been minimized.

@carstene1ns

carstene1ns Jul 9, 2018

Member

I do not really understand why we need the double refresh here? Can this not be shuffled around?

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

I also think this looks more like a hack then a solution

@@ -250,6 +253,7 @@ void Scene_Map::FinishTeleportPlayer() {
void Scene_Map::CallBattle() {
Main_Data::game_data.system.before_battle_music = Game_System::GetCurrentBGM();
Game_System::SePlay(Game_System::GetSystemSE(Game_System::SFX_BeginBattle));
Game_System::BgmPlay(Game_System::GetSystemBGM(Game_System::BGM_Battle));

This comment has been minimized.

@carstene1ns

carstene1ns Jul 9, 2018

Member

Why is this needed? Scene_Battle::Start() will start the music too.

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

Because the battle music already starts when the screen flash appears on the map.
The call in Start() is still needed for the BattleTest but it should be in an extra commit because it has nothing to do with GameOver.

This comment has been minimized.

@Albeleon

Albeleon Jul 9, 2018

Member

That was a mistake on my part, I forgot to take it away when putting the commit, my bad. I'll update it and take it away.

@@ -246,6 +247,9 @@ namespace Game_System {
void OnBgmReady(FileRequestResult* result);
void OnSeReady(FileRequestResult* result, int volume, int tempo, bool stop_sounds);
bool IsSoundStopFilename(RPG::Sound sound);
const RPG::Sound* GetFirstSound(RPG::Animation *animation);
}

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

Imo this whole commit is overcomplecating the problem.
The only thing you want to do is: Play the first SE of the animation, otherwise nothing.

So why don't you add a new function void SePlay(const RPG::Animation& animation) which simply delegates the work to the other existing SePlay. Then you can also remove IsSoundStopFilename

/**
 * Plays the first sound effect of an animation if it has one.

This comment has been minimized.

@Albeleon

Albeleon Jul 9, 2018

Member

This doesn't solve the problem, which is "if it's a sound that is not (OFF) or similars, stop looking at more sound effect animations", and there's no way to know by executing SePlay if the sound was (OFF) or not. Remember that every effect executes the sound "(OFF)". That's why I need some kind of check. And I didn't want to use the default IsStopFilename because that searches for the files and all that, and that could be not efficient, specially in a function that is looping until it find a valid sound.

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

The filenames are cached so calling the FileFinder hundreds of times is fine but I didn't consider how finding the first SE works, so have to rethink this.

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

well I would prefer a SePlay(const RPG::Animation& animation) which uses the normal "IsStopFilename" func that already exists and then delegates the work to SePlay when it's not a StopFilename.
This will fail for a small corner case in emscripten (SE start/ends with ( / ) ) but I don't see a good way to solve this for this.

Game_System::SePlay(Game_System::GetSystemSE(Game_System::SFX_UseItem));
const RPG::Sound* sound = Game_System::GetFirstSound(ReaderUtil::GetElement(Data::animations, ReaderUtil::GetElement(Data::skills, id)->animation_id));
if (sound != nullptr)
Game_System::SePlay(*sound);

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

This needs sanity checks because this is external data which is untrusted.
Look at other places where ReaderUtil::GetElement is used and add similiar Output::Warning handling.
At least for the animation because the skill is already sanitized in UseSkill()

This comment has been minimized.

@Albeleon

Albeleon Jul 9, 2018

Member

GetFirstSound() already checks if the animation is "nullptr" and returns a "nullptr" sound in that case. But if you still prefer that I make a sanity check for animation, then I'll do it.

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

yes pls do a check for a valid animation beforehand imo functions where NULL is just an invalid argument the argument type should be reference & and not pointer *.

@@ -103,6 +102,9 @@ class Scene_Title : public Scene {
/** Contains the state of continue button. */
bool continue_enabled;
/** If it returns to the scene from Scene_Load, check this to do a quick transition */
bool quick_transition;

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

I really don't like this commit.
It is very invasive considering the only purpose is a minor problem which is restarting the BGM when going from load to title or did I miss something else?

This comment has been minimized.

@Albeleon

Albeleon Jul 9, 2018

Member

It's not only the BGM, it's not cleaning the cache of all the data and restart the Scene_Title from the beginning, and also, make sure that the TransitionIn from Title->Load to Title is a simple quick one, instead of the slow FadeIn. The only way I found for Scene_Title to detect this case was using a new "bool" (we cannot reuse "restart_title_cache").

RPG::Item* equipment2 = ReaderUtil::GetElement(Data::items, actor->GetShieldId());
if (equipment1 && !equipment2 && !equipment1->two_handed && !new_equipment->two_handed) {
actor->ChangeEquipment(slot + 1, item_id);

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

Is there a way to do all this logic in actor->ChangeEquipment? In the previous code the only thing the interpreter did was delegating the work to the actor class.
Also these "return true" are wrong because you are in a for-loop, use "continue".

This comment has been minimized.

@Albeleon

Albeleon Jul 9, 2018

Member

Oops, the "return -> continue" is right, I'll correct it

If we do this in actor->ChangeEquipment, then it will affect too if we change weapons from the menu.
i.e. I'm in the Equipment menu, my character with two weapons have only the first one equipped, and I just want to change that one, that's it. If I put that inside ChangeEquipment, then instead of replacing the old weapon for the new one, it will just insert the new one in the second slot and let the first one as it is.
This is a feature only the Event Command does.

@carstene1ns carstene1ns added this to the 0.5.4 milestone Jul 9, 2018

*
* @param animation animation data.
*/
void SePlay(RPG::Animation &animation);

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

can be const RPG::Animation&

@@ -121,7 +121,13 @@ void Scene_ActorTarget::UpdateSkill() {
return;
}
if (Main_Data::game_party->UseSkill(id, actor, target_window->GetActor())) {
Game_System::SePlay(Game_System::GetSystemSE(Game_System::SFX_UseItem));
RPG::Skill *skill = ReaderUtil::GetElement(Data::skills, id);
RPG::Animation *animation = ReaderUtil::GetElement(Data::animations, skill->animation_id);

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

code style: The pointer belongs to the type: RPG::Skill* (same for the & in SePlay)

@@ -48,7 +48,7 @@ void Window_TargetStatus::Refresh() {
str = Utils::ToString(Main_Data::game_party->GetItemCount(id));
} else {
const RPG::Skill* skill = ReaderUtil::GetElement(Data::skills, id);
str = skill->sp_cost;
str = Utils::ToString(skill->sp_cost);

This comment has been minimized.

@Ghabry

Ghabry Jul 9, 2018

Member

You said this is a RPG_RT bug but imo we should call here CalculateSpCost to show the real value

RPG::Animation* animation = ReaderUtil::GetElement(Data::animations, skill->animation_id);
if (!animation) {
Output::Warning("UpdateSkill: Skill %d references invalid animation %d", id, skill->animation_id);
return;

This comment has been minimized.

@Ghabry

Ghabry Jul 10, 2018

Member

This return is not ideal because the func executes more important code after

std::string path;
std::vector<RPG::AnimationTiming>::const_iterator it = animation.timings.begin();
for (; it != animation.timings.end(); ++it) {
if (!isStopFilename(it->se.name, FileFinder::FindSound, path) && !path.empty()) {

This comment has been minimized.

@Ghabry

Ghabry Jul 10, 2018

Member

this can be replaced with a for(const auto& anim)-loop

The path.empty check is not necessary, isStopFilename already ensures non-empty paths when returning true

@@ -46,13 +46,15 @@ class Window_TargetStatus : public Window_Base {
* @param id ID of item/skill.
* @param is_item true if ID for an item, otherwise for a skill.

This comment has been minimized.

@Ghabry

Ghabry Jul 11, 2018

Member

needs @param for actor_index

@@ -254,12 +260,27 @@ void Game_Actor::ChangeEquipment(int equip_type, int item_id) {
if (item_id != 0) {
Main_Data::game_party->RemoveItem(item_id, 1);
}
const RPG::Item* item = ReaderUtil::GetElement(Data::items, GetWeaponId());
const RPG::Item* item2 = ReaderUtil::GetElement(Data::items, GetShieldId());

This comment has been minimized.

@Ghabry

Ghabry Jul 11, 2018

Member

Could you add a comment here that this unequips the other item when a two-handed item is equipped?

@Ghabry

Ghabry approved these changes Jul 11, 2018

premature approval, only have these 2 minor "complaints"

@@ -423,6 +423,13 @@ class Game_Actor : public Game_Battler {
*/
const std::vector<int16_t>& GetWholeEquipment() const;
/**
* Checks if the actor has a specific item equipped.
*

This comment has been minimized.

@carstene1ns

carstene1ns Jul 11, 2018

Member

@param equip_id missing

@Albeleon Albeleon referenced this pull request Jul 12, 2018

Open

Ascendence: Visual bugs #1293

2 of 4 tasks complete

Albeleon added some commits Jul 8, 2018

Implement two-handed weapons and reactions.
Solution: For two-handed weapons:
 - When the ChangeEquipment function is used (whether manually in the menu or by EventCommand), after being equipped, it checks whether one of the weapon/s or shield is two-handed. In that case, it removes the other from the equipment.
 - When selecting a weapon that would cause the other shield/weapon to drop because of being two-handed, it should be reflected on the predictive stats (if "Sword +10 ATK two-handed" equipped, and want to equip "Shield +5 DEF, the predictive results should show -10 ATK -5 DEF). We change Scene_Equip to account for this case.
Solve CommandChangeEquipment particular cases.
Solution:
 - In case you try to add a shield from a character with two weapons, it won't do anything. Removing a weapon will only take away the first weapon, removing a shield will only take away the shield or the second weapon.
 - In case you don't have any particular equipment or the character doesn't have any equipped, it adds one to the inventory. To know if the particular character has an item equipped, we create the function Game_Actor::IsEquipped.
 - In case the character has two weapons, it only has the first one equipped, and both this and the new weapon are not two-handed, then it equips this new weapon in the second slot.
Generate first sound of skill animation when apply in Menu.
Solution: To generate the sound, we create a function in Game_System::SePlay() that takes an animation and only plays the first sound it finds that isn't a StopFilename. Then in Scene_ActorTarget we execute it. If there is no sound, it doesn't sound anything.
When going to Scene_Title, not restart when in Title->Load Menu.
Solution:
 - We create a Game_Temp::restart_title_cache, false by default, that will be true when a new game starts or a file has been loaded. When you press F12, restart_title_cache will be set to false even if you are in that load menu.
 - In Continue(), only if restart_title_cache is true, then the cache will be cleaned and the scene restarted. Otherwise, it just resumes normally.
 - Since when the Title screen is restarted the window_command's visibility is set to false, and we need it to be true when just continuing, the Suspend() function is unnecessary.
 - To create a quick transition from Title->Load to Title (if you press CANCEL), we check if "window_command->GetVisible() == true" (check the previous point). Otherwise, it executes the other transitions. We also eliminate a weird Transition::Init to Erase that does nothing because it's inmediately changed by the next functions.
Help message from Scene_Equip should be loaded before the TransitionI…
…n starts.

Solution: Moving SetHelpWindows after each item_window has been set solves the problem.
When TransitionIn in a Scene_File with default slot 1-3, not draw all…
… cursors.

Solution: This error seems to happen when Refresh hasn't been executed the first time before the loops of file_windows[i]->Update(). To solve this, we move in Start() the Refresh() before the Update().
GameOver has long FadeIn and Outs. When GameOver from Map, it fades o…
…ut always.

Solution: Add TransitionIn/Out to Gameover with a long time (80 for example). When TransitionOut in Scene_Map, if the current instance is GameOver then fadeout.
Show in Scene_ActorTarget the SP cost correctly.
Problem: The SP in Scene_ActorTarget (when you throw in the menu a skill to a character) showed random characters. It happened because the int value was directly put into a string instead of using Utils::ToString (like the itemCount uses).
Make Game_Actor::IsItemUsable account for hero classes in 2k3.
Solution: If it's Rpg2k3, consider the hero class with similar conditions to the actor_set.

An item class_set has as first element [0] "None", which means this happens if the actor has no class (GetClass returns nullptr). This also means the first class is in position [1] instead of [0], like an one-offset. This is only appliable to this particular "class_set".
Show in Scene_Equip the right item_weapon list before TransitionIn.
Problem: When initializing Scene_Equip, all the item_weapons are visible, so the last one visible is the accessory ones, and before TransitionIn ends that's what it always showed. We change this by setting the visibility already.
Show CursorRect in Window_Shop before TransitionIn.
Solution: Add CursorRect() in Start().

Albeleon added some commits Jul 15, 2018

When in Airship, damaged Terrain makes no damage.
Solution: Put "!InAirship()" as condition.
Associate translation from frame to 0.1s to Game_Interpreter_Map.
Problem: Flashes, Tints and Shakes are always translated from frames to tenths (0.1s) except in battler sprites, which is a problem for battle animations that send the same amount of frames for both. This made screen flashes in battle being longer and battler flashes being shorter.

Solution: To be consistent, since Game_Interpreter is the only that takes "tenth" values, it will be the manager of translating from frames to tenths. All Game_Interpreter operations will do the "DEFAULT_FPS / 10" instead of Game_Character and Game_Screen. And since now it works by frames, the Game_Player red flash when damaged terrain is 6 (equivalent to 0.1s) instead of 1.
if (!hero->PreventsTerrainDamage()) {
red_flash = true;
hero->ChangeHp(-std::max<int>(0, std::min<int>(terrain->damage, hero->GetHp() - 1)));
}
}

This comment has been minimized.

@Ghabry

Ghabry Jul 29, 2018

Member

This fix made me curious if the other vehicles play "footstep sounds": They do. Which means footstep sound is a bad naming in the editor *g*

else {
SetY(4);
}
SetZ(Priority_Timer);

This comment has been minimized.

@Ghabry

Ghabry Jul 29, 2018

Member

I don't like the solution but great that you found this bug. This affects everything when returning from battle, e.g. "Change Vehicle Location" in a battle will show the vehicle at the wrong position during fade in.
The correct way to fix this is the same one that solved the same issue for Teleports:

Add spriteset->Update(); to the end of Continue().

@Ghabry

This comment has been minimized.

Member

Ghabry commented Jul 29, 2018

(for everything else I give my approval)

Change the SetY value from a Timer before TransitionIn.
Problem: A timer that goes from battle to map and viceversa will show the old Y position until TransitionIn ends, because the SetValue is in Update().

Solution: Update spriteset in Continue
@Ghabry

Ghabry approved these changes Sep 19, 2018

@carstene1ns carstene1ns merged commit 46ac994 into EasyRPG:master Sep 30, 2018

7 checks passed

Android (armeabi-v7a) Build finished.
Details
GCW0 Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment