-
Notifications
You must be signed in to change notification settings - Fork 98
Wipe parsing #1053
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
base: dev
Are you sure you want to change the base?
Wipe parsing #1053
Conversation
microlith57
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few stylistic concerns but looks good!
| /// Mark this renderer as a custom <see cref="ScreenWipe"/> with an identifier.<br/> | ||
| /// If there is no match, then the full type name of the Screen Wipe is checked for. | ||
| /// </summary> | ||
| /// <param name="ids">A list of unique identifiers for this Screen Wipe.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: might be good to note that this follows the ID[=MethodName] pattern!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do the same for the other attributes too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, i guess i neglected to check those :p
yea if you can add wording to their docs too that'd be good!
| namespace Celeste { | ||
| public class patch_AreaData : AreaData { | ||
|
|
||
| public static readonly Dictionary<string, Action<Scene, bool, Action>> WipeLoaders = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: can the Action<Scene, bool, Action> be a named delegate type instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may just be my experience, but getting an Action and a delegate to work together like this is kind of tricky.
so I thought just using an Action would be easier
Celeste.Mod.mm/Mod/Meta/MapMeta.cs
Outdated
| if (Everest.Events.MapMeta.ParseWipe(wipeStr) is { } wipeLoader | ||
| || (patch_AreaData.WipeLoaders.TryGetValue(wipeStr, out wipeLoader) && wipeLoader is not null)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: can this be split into an if/else if? it feels a bit cramped as a single condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, will do
this PR adds some alternative ways for modders to apply their Screen Wipes to a map.
this mainly serves to give unique identifiers to the Screen Wipes, so that the game doesn't have to check for their full type names, and their classes can be freely reorganised if needed.
Everest.Events.MapMeta.OnParseWipeaccepts a method that returns anAction<Scene, bool, Action>and takes the argumentstring wipe, which is the value of the current Wipe metadata being parsed.the parameters of the returned
Actionare similar to the constructor for a Screen Wipe:(Scene scene, bool wipeIn, Action onComplete).the
[CustomWipe]attribute should be placed on a class that extendsScreenWipe, and is used to give it a unique identifier. the class requires one of the following in order to be registered as a custom wipe:public staticgenerator method (calledLoadby default) that returnsScreenWipeand has the signature(Scene scene, bool wipeIn, Action onComplete), or...(Scene scene, bool wipeIn, Action onComplete).