Skip to content
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

Add Maniac Patch Event commands #271

Merged
merged 1 commit into from
Sep 26, 2019
Merged

Conversation

Ghabry
Copy link
Member

@Ghabry Ghabry commented Oct 30, 2018

SetMousePositon surprisingly doesn't add a new chunk to the savegame, which means the offset is not stored.

Not fully sure what "EndLoading" is supposed to do, doesn't match with the documentation. waiting for a response on twitter.

@carstene1ns
Copy link
Member

I think this should involve a generator change that applies some "Maniac Patch" comment here.

@Ghabry
Copy link
Member Author

Ghabry commented Oct 30, 2018

jadehpy wrote me that End Loading reduces the blackscreen after load from 6 frames to 1 frame but the blackscreen here is around 0.5s long, so I can't really notice this 5 frame difference o.O.

@mateofio
Copy link
Contributor

I agree with carstene1ns, these need to clearly be marked as non-standard in some way.

Also #272

@Ghabry
Copy link
Member Author

Ghabry commented Oct 30, 2018

I agree with your objections.

Note that this patch fulfills the "doesn't conflict with normal games" policy, otherwise I wouldn't even consider adding it.

@mateofio
Copy link
Contributor

Maybe even a separate fields_maniac.csv or something like that?

@Ghabry
Copy link
Member Author

Ghabry commented Oct 30, 2018

I find these commands so useful that I would even consider them for the EasyRPG Editor.
Which spawns the discussion where to put (potentially) new Event commands invented by us.

In this case enums_maniac.csv (no new fields) and *_easyrpg.csv feels more future proof.

@mateofio
Copy link
Contributor

mateofio commented Oct 30, 2018

*_easyrpg.csv makes sense to me for custom stuff.

In the long term, we need to decide on some uber-format to support all patches, but there is no reason we can't use exactly the maniac convention to start with. If some other patch implements the same thing we'll just have to translate it at the liblcf level to how maniac does it.

@mateofio
Copy link
Contributor

mateofio commented Nov 2, 2018

For EventCommand::Save, what is the value of SaveSystem::screen in some of those maniac saved games?

Normally in RPG_RT, all saving goes through the save menu, and therefore in save games screen == 5 (filemenu) always.

If this command lets you save anywhere, I'd bet screen == 0 (map). Can you save in battles?

@Ghabry
Copy link
Member Author

Ghabry commented Nov 4, 2018

When saving in Battles the screen is "3" and loading them puts you on the map. You can even save in BattleTests (but you can't load it).
Loading these saves corrupts something, RPG_RT crashes when closing.

Edit: Moving it to *_maniac.csv is surprisingly difficult because the generator needs many updates o.O

@Ghabry
Copy link
Member Author

Ghabry commented Nov 5, 2018

new solution. opinion?

@mateofio
Copy link
Contributor

mateofio commented Nov 5, 2018

My vote is for enums_easyrpg.csv instead of having separate files for different patches.

In other words, we have base RPG_RT compatibility layer, and then our easyrpg customization layer which also incorporates support for patches (with data transforms if needed).

@Ghabry
Copy link
Member Author

Ghabry commented Nov 5, 2018

I'm okay with this solution 👍

@mateofio
Copy link
Contributor

mateofio commented Nov 6, 2018

Apologize for being so nitpicky, but if they are easyrpg_enums.csv then I think the enum tags themself should also be easyrpg and not Maniac_.

How about an EZ_ prefix? Like EZ_Save? Or EasyRPG_Save if you want to be longwinded.

@Ghabry
Copy link
Member Author

Ghabry commented Nov 6, 2018

Well the problem is that these event commands are specific to Maniac Patch and we decided to only have one file.

I would be fine with "EZ_", doesn't really matter for me.
Waiting for @fdelapena and @carstene1ns favourite, then I will choose the winner. xD

@mateofio
Copy link
Contributor

mateofio commented Nov 6, 2018

To give more context of where I am coming from, I'm thinking to make a PR add these custom equipment settings. This is why I made equipment_setting an enum and not a bool.

EZ_EquipmentSetting_either
EZ_EquipmentSetting_both

(where EZ can be whatever convention we decide)

That would be a pure EasyRPG extension.

Yours are specific to Maniac but I see them as EasyRPG extensions first, which also happen to be compatible with maniac patch.

@fdelapena
Copy link
Contributor

I agree once EasyRPG Editor adopts this feature as own, keeping the patch name prefix may not be worth, adding a comment or documentation somewhere about the origin and design decision of those numeric IDs is still useful, though.

@fdelapena fdelapena assigned fdelapena and unassigned fdelapena Nov 8, 2018
Copy link
Member

@carstene1ns carstene1ns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved now, can merge after rebase :P

@Ghabry
Copy link
Member Author

Ghabry commented Dec 8, 2018

soooooo, what is the result? Do you want Maniac, EZ or Easy-prefix now?

@carstene1ns
Copy link
Member

Since all of these additions are good ideas to support later, should be okay to use our prefix, but may be a good idea to clarify the origin with the current maniac prefix.

@mateofio
Copy link
Contributor

mateofio commented Dec 9, 2018

Why don't we have 2 conceptual name spaces?

EasyRPG and Maniac.

Each custom command that's compatible with a patch shall have an alias in that patch's unique namespace. It also needs a id in the EasyRPG namespace in order to be compatible with our tools.

The right place to store information about patch lineage is in the code itself. This will also be the foundation of parsing logic needed later to convert incompatible map patch customization to EasyRPG.

@fdelapena fdelapena modified the milestones: 0.6.0, 0.6.x Feb 12, 2019
@mateofio
Copy link
Contributor

mateofio commented Feb 21, 2019

Regarding this save command. How does it work? Does it save immediately or does it wait till the end of the frame and save at the same time opensavemenu would be called?

If it's the former then I don't support adding this command. It's useful for debugging and research for sure, but there are many local variables mid frame which don't have LSD chunks.

One that comes to mind is the loop counter in the interpreter that goes up to 10000.

The result will be heisenbugs with broken saves done at the wrong time. They would so rare and hard to track that fixing them would be next to impossible. I would never use such a feature in a game as practically it would just result in game instability and corrupted save games.

I'm not convinced the maniac author has thought this through carefully.

@Ghabry
Copy link
Member Author

Ghabry commented Feb 21, 2019

That's a good point, unfortunately there is no event tracer for 2k3 Steam due to lack of DynRPG to figure this out easily.
I would expect an interpreter yield, otherwise this would be really a bad design choice, will try to figure this out.

@Ghabry
Copy link
Member Author

Ghabry commented Feb 21, 2019

While testing this I directly found a bug with the normal scene calling:

Parallel Process 1:

Loop
 V1 += 1
 if V1 > 1000
  Save
 endif
Repeat

PP2:

Message: V[1]

A blank non-transparent message box appears right before it fades to the save scene and after closing the scene it draws the text. The next boxes are all correct. Must be a problem with initialisation and fade, is fixed by a Wait 0.0.

@fmatthew5876 You were right.
I simply use the "execution limit" of Parallel Processes to test this: I save and look at the message box after loading:
The output is 3000 (Maniac) vs 5000 (Scene) which means it doesn't use the same way because the output is different.

I still like this command but we can't support it as fully intended (but was likely just not properly thought about a said)

@mateofio
Copy link
Contributor

Here's another,

Parallel events are allowed to continue running while a message box is up. So you could save your game while a message box is active.

I'd like to see what happens when you try to load that save..

@fdelapena fdelapena modified the milestones: 0.6.1, 0.6.2 Jun 30, 2019
@Ghabry
Copy link
Member Author

Ghabry commented Sep 23, 2019

Finally rebased it and added all commands up to version 190920

@fdelapena fdelapena merged commit 8324355 into EasyRPG:master Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants