FireEmblemUniverse / ColorzCore

A rewriting of Core.exe for EA.
GNU General Public License v3.0
7 stars 7 forks source link

Add new features (April~May 2024) #63

Closed StanHash closed 19 hours ago

StanHash commented 5 months ago

I've been hacking new features into ColorzCore for the past few weeks or so. This is getting close to ready.

I release binaries for this branch whenever here: https://github.com/StanHash/ColorzCore/releases.

Features

Some of these were suggested by other users.

Offsets are now Addresses

New operators

Directives and Macros

Formatted Interpolated strings

ReadByteAt

STRING and custom .TBL encodings

Misc.

Diagnostics

Bug fixes

Technical changes

Uncertain design points

Testing script

In the Tests directory lies a single python script named run_tests.py. I have been working on this very basic test program in an attempt of making sure I (and possibly future contributors) don't break anything important as new stuff gets added.

Things that break

Warnings have been introduced to try and diagnose most of these.

Crazycolorz5 commented 5 months ago

This looks very impressive and well documented. I can merge this when you feel it's ready, though lemme give my 2 cents on a few things that you're (or I'm) unsure of:

Add ReadByteAt(offset), ReadShortAt(offset) and ReadWordAt(offset) built-in macros. They allow reading data from the working binary. It only sees what was there before assembly. Those macros are disabled in AA mode.

This makes me very uncomfortable. I think a strong point of the EA language is that it's agnostic to the file being edited. That said, AA mode is the default (correct me if I'm wrong?) -- which I guess then begs the further question of what would this be used for? It's not inherently bad but I think there's a thing to be thought of for complexity bloat and this could lead to very unintuitive conditionals being written that would be better to be handled elsewhere in tooling or have some thought put into it to avoid.

'Maybe' has been removed in favor of extensions that work with nullable types (for example: IfJust works on any 'T?') for reference types, this works on any type (as nullable for reference types is just an annotation)

I don't really mind this if it's functionally equivalent and the nullable annotation on references is enforced by the type system. The only thing we really lose is the ability to next the constructor (e.g. Maybe<Maybe<Int>>) but that's not really useful if we have bind anyway.

The ?? operator seems like it would be redundant with IsSymbolDefined. However, they both have different semantics: a ?? expression is always evaluated at the very end of assembly (when data is inserted) while IsSymbolDefined is evaluated immediately, so it's not that simple. I wonder if there's something that can be done here to make this more elegant regardless.

My reading of your documentation makes me think of ?? as being useful for preprocessor stuff (or vice-versa). Since some preprocessor stuff care about immediate evaluation (for parameter passing) I can see having both be useful.

All Read...At macros could be defined in terms of any one other. I don't know if we want to keep all three or just one and let the user add their own.

Keep all of them as language defined. Same feel as BYTE SHORT WORD being definable in terms of BYTE, but having all 3 just makes it nicer.

the ternary operator A ? B : C is not implemented, but (A && B) || C would be functionally equivalent

This makes me deeply uncomfortable and I am honestly not very on board with && and || making sense for how preprocessors would handle things, but not with how a language would handle things, but I think your definitions are sensible so I won't object to this.

In parallel to this, I have been working on a very basic test program and set, in an attempt in making sure I (and possibly future contributors) don't break anything important. Idk if I should include this here or no.

Test cases/scripts should be included with the project, please do include them here if you have them.

Also I admittedly have not been hacking or maintaining this as much in recent years, so ... don't expect the most thorough of reviews or tests for functionality or any grand visions for design. Comments I give above are mostly just my opinions / what I've seen as commonly done in project/language design. If the people using this want certain features and they're implemented, I'll merge it, the most I'll do is bugfixing here and there.

StanHash commented 5 months ago

AA mode is the default (correct me if I'm wrong?)

AA is the mode that outputs ASM+LDS that was contributed a while ago I don't think it sees much use beyond the one user. The default mode is just A, so the Read...At macros would be enabled basically always, I just thought I would note the technicality.

The ability to check existing values in the binary was brought up when I asked for suggestions (by Vesly). I was not a fan of it conceptually either but I do understand the potential uses (I believe Vesly is working on randomizer mods to be applied to existing mods? I which case this could be used to check whether some known patch was installed or no).

I was thinking making it require enabling the macros explicitly (probably through a command line flag) but couldn't reasonably convince myself it wouldn't just be annoying.

StanHash commented 5 months ago

Last three things I want to (attempt to) implement before freezing this feature-wise:

Then I will do some larger scale testing (I've been periodically testing on SkillSystem to make sure nothing breaks too horribly but I'm looking to test on other (perhaps more involved) public buildfiles) and if nothing unexpected breaks this will be ready.

I guess a friendlier summary of changes could also be on the TODO list.

StanHash commented 5 months ago

This is ready feature-wise. Any further changes would be bugfixes.

Tested on Skill System and Pokemblem. Skill System builds with no changes, and Pokemblem (after addressing the diagnosed breakages) builds with some changes that relate to bugfixes which don't (seem to) break anything.

I also put up a build here: https://github.com/StanHash/ColorzCore/releases/tag/20240505

Crazycolorz5 commented 19 hours ago

Sorry, I must've missed the email saying the changes were review-ready! This is good to merge, and I'll update release posts with the updated build.