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

AURORA: New FEV loading capabilities #367

Closed
wants to merge 2 commits into from

Conversation

Nostritius
Copy link
Contributor

Add some new values which can now be read from the FEVFile loader

uint32 type;
int32 integerValue;
float floatValue;
Common::UString stringValue;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that same pattern, a variable with multiple potential types, comes up again and again in the codebase now. In the ActionScript code, the NWScript code, the Lua code, etc.

Maybe be should see if we can use Boost.Any or Boost.Variant for this instead? This code might be the ideal test environment for that. Could you see if you can do that?

For the differences between Any and Variant, see https://www.boost.org/doc/libs/1_53_0/doc/html/variant/misc.html#variant.versus-any . I'd say Variant sounds like the better candidate, with Any a fall-back if Variant should not work for our use case.

You probably still need variable to store the current type, though. I'd rather that be an enum instead of just an integer without any obvious meaning, too.

@Nostritius Nostritius force-pushed the aurora_fevproperties branch 2 times, most recently from 12d4a81 to 0f8db23 Compare September 23, 2018 17:25
@Nostritius
Copy link
Contributor Author

I have now changed the properties to use an enum and boost::variant. As for the other occasions with such constructs (at least the ones I wrote), I will change them in other PRs.

Property property;
property.type = PropertyType(fev.readUint32LE());
switch (property.type) {
case 0: // Integer value
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace these with the enum value as well, please?

@Nostritius
Copy link
Contributor Author

Done

@DrMcCoy
Copy link
Member

DrMcCoy commented Nov 3, 2018

Merged as 204cccc...29ce633, thanks! :)

@DrMcCoy DrMcCoy closed this Nov 3, 2018
@DrMcCoy
Copy link
Member

DrMcCoy commented Nov 18, 2018

Btw, upon further thought, I think this belongs into sound/, not into aurora/.

@Nostritius
Copy link
Contributor Author

Hmm, but SSFFile is also in aurora?

@DrMcCoy
Copy link
Member

DrMcCoy commented Nov 19, 2018 via email

@Nostritius
Copy link
Contributor Author

Ok, i will move this in my next update for FEV files

@Nostritius Nostritius deleted the aurora_fevproperties branch December 2, 2018 14:51
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.

None yet

2 participants