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

Improvements to Panel Parsers #597

Closed
wants to merge 5 commits into from
Closed

Improvements to Panel Parsers #597

wants to merge 5 commits into from

Conversation

leezer3
Copy link
Owner

@leezer3 leezer3 commented Mar 24, 2021

The panel.cfg and panel2.cfg parsers both make heavy use of text comparisons to determine the section / key within the section.
These can be converted into enum values for much better readability.

n.b.
This takes advantage of the C# feature that 2 members of an enum may have the same integral value. When converted to bytecode, the integral value is used as opposed to the textual representation.
https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1069
May need disabling if strict code analysis is enabled (it doesn't trip ReSharper or the default VS2019 rules), or possibly the way duplicate entries are declared to match Microsoft's preferences, although personally I think it's better this way.

CHECK / TODO:

  • Very easy to get this wrong, needs testing to ensure that we don't get a duplicate somewhere and produce really unexpected results.
  • Any further not found variants?
  • Port to Panel2 parser.

@leezer3
Copy link
Owner Author

leezer3 commented Mar 26, 2021

Now in the Panel2 parser.
I'd quite like to use a similar enum for the animation subjects too.
This is slightly more complex, as ATS subjects use a numerical suffix. I suppose we could split the string or something, need to think about that....

@leezer3
Copy link
Owner Author

leezer3 commented Apr 7, 2021

Rebased and squashed.

#544

Integrating this would really help too, in making everything a lot more readable.
Trouble is that that I've never been quite happy with the way that works in terms of the number of parameters passed for error reporting purposes, hence not merging...

The essential trouble is that BVE has always been incredibly loose with it's number parsing, and just spits out an error, whilst it'd probably be more conventional to throw an error.
I suppose we could use a single tryparse method and then the error message within the parsers as per now, but this would lose us the specific 'wrong' argument.

Another option might be a wrapper struct e.g.

strut ParseResult
{
       Vector3 Result;
       bool errorX;
       bool errorY;
       bool errorZ;
}

This seems like a complete waste of a struct to me though :/

@leezer3
Copy link
Owner Author

leezer3 commented Apr 10, 2021

Spent this morning playing with a block based parser, based heavily on my existing block based implementation for use in the internal formats.
Only done XML at the minute as an initial test on the main panel block.

This initially seems to work quite well, although it uses generics a little too much for my taste & would require the sound parser keys etc. converting int enums to use everywhere.
It also requires C# 7.3 (ought to be available in our Mono versions as Mono should get this as of 5.16), which doesn't seem to be available on AppVeyor.

Azure build failure seems unrelated (git checkout failed) & this isn't finished enough to bother kicking it.
Might merge without the block parser and dump that in another PR.

@leezer3
Copy link
Owner Author

leezer3 commented Apr 10, 2021

Hmm, do we need a langversion tag?
dotnet/docs#10825

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant