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

Move voxel loader to Mods.Cnc. #13485

Merged
merged 9 commits into from Jun 14, 2017

Conversation

Projects
None yet
3 participants
@pchote
Member

pchote commented Jun 9, 2017

One of the big focuses of the next release has been decentralizing our engine so that modders aren't restricted by our default engine/mods install. #13223 fell nicely under this umbrella point, leaving the voxel code as the last hardcoded format, and also the last piece of reverse-engineered proprietary logic in the core engine.

This PR fixes that by separating our voxel code into a generic model loading interface in the engine, and moving the voxel-specific code to Mods.Cnc.dll. Once this has been merged we can proudly announce that our core engine is 100% free / clean code 🎉. This also makes it a lot easier to implement #4689 at some point in the future.

This isn't nearly as intimidating as it looks, and should be straightforward to review if you go commit by commit (but note that github has messed up the display order).
The only real changes are in:

  • "Add backend plumbing for model loaders.": implements new backend for defining and creating the mod-level model loaders and model sequence merging.
  • "Move Voxel code to Mods.Cnc.": Ports the old VoxelProvider code over to the new VoxelModelSequenceLoader.

Everything else is just renaming or moving code.

The old code relied on static state, with ModData.InitializeLoaders manually clearing and reloading the state as part of the map load. The new code has ModelCache owned by the World, which gives the same lifetime as before but without the ugly manual management.

I have not written an upgrade rule for the VoxelSequencesModelSequences map yaml rename because that would mean bumping the map format for a feature that we don't use at all in our default mods and which we don't officially support yet for modders.

I will rename the WithVoxel* traits in a followup PR to reduce the rebase conflicts that come with renaming and upgrade rules.

@pchote pchote added this to the Playtest featuring updated HitShapes milestone Jun 9, 2017

@pchote pchote referenced this pull request Jun 9, 2017

Open

OutOfMemory #12494

@rob-v

Minor issue, questions.

@@ -29,6 +29,7 @@ public class Ruleset
public readonly IReadOnlyDictionary<string, MusicInfo> Music;
public readonly TileSet TileSet;
public readonly SequenceProvider Sequences;
public readonly IReadOnlyDictionary<string, MiniYamlNode> ModelSequences;

This comment has been minimized.

@rob-v

rob-v Jun 11, 2017

Contributor

It is better MiniYamlNode than e.g. SequenceInfo like for other sections (MusicInfo, ActorInfo,...)?

@rob-v

rob-v Jun 11, 2017

Contributor

It is better MiniYamlNode than e.g. SequenceInfo like for other sections (MusicInfo, ActorInfo,...)?

This comment has been minimized.

@pchote

pchote Jun 11, 2017

Member

We don't have a *Info for this: it is up to the mod-defined parser to parse what it wants from the yaml tree.

@pchote

pchote Jun 11, 2017

Member

We don't have a *Info for this: it is up to the mod-defined parser to parse what it wants from the yaml tree.

ruleset = new Ruleset(actors, weapons, voices, notifications, music, ts, sequences);
var modelSequences = dr.ModelSequences;
if (mapModelSequences != null)
modelSequences = MergeOrDefault("ModelSequences", fileSystem, m.ModelSequences, mapModelSequences, dr.ModelSequences,

This comment has been minimized.

@rob-v

rob-v Jun 11, 2017

Contributor

You can reuse here modelSequences for dr.ModelSequences.

@rob-v

rob-v Jun 11, 2017

Contributor

You can reuse here modelSequences for dr.ModelSequences.

This comment has been minimized.

@pchote

pchote Jun 11, 2017

Member

I'd like to keep it separate to make it clear that it is replacing modelSequences, not modifying it. This is really just a terniary statement, but the MergeOrDefault branch is too ugly to write it as one.

@pchote

pchote Jun 11, 2017

Member

I'd like to keep it separate to make it clear that it is replacing modelSequences, not modifying it. This is really just a terniary statement, but the MergeOrDefault branch is too ugly to write it as one.

Show outdated Hide outdated OpenRA.Mods.Cnc/Graphics/Voxel.cs
@@ -76,7 +80,7 @@ public float[] TransformationMatrix(uint limb, uint frame)
return t;
}
public VoxelRenderData RenderData(uint limb)
public ModelRenderData RenderData(uint limb)

This comment has been minimized.

@rob-v

rob-v Jun 11, 2017

Contributor

small nit: it is ok, could be changed to match interface signature: RenderData(uint section)
(also for float[] TransformationMatrix(uint section, uint frame))

@rob-v

rob-v Jun 11, 2017

Contributor

small nit: it is ok, could be changed to match interface signature: RenderData(uint section)
(also for float[] TransformationMatrix(uint section, uint frame))

This comment has been minimized.

@pchote

pchote Jun 11, 2017

Member

I'm not sure about this: in the voxel file format sections are called limbs, so we can't avoid having a naming clash somewhere. This seems like a better place to have it than at e.g. sectionData = new Limb[vxl.LimbCount]

@pchote

pchote Jun 11, 2017

Member

I'm not sure about this: in the voxel file format sections are called limbs, so we can't avoid having a naming clash somewhere. This seems like a better place to have it than at e.g. sectionData = new Limb[vxl.LimbCount]

@pchote pchote added PR: Needs +2 and removed PR: Needs +2 labels Jun 11, 2017

@rob-v

rob-v approved these changes Jun 11, 2017

👍

@reaperrr reaperrr merged commit 9c9a23b into OpenRA:bleed Jun 14, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pchote pchote deleted the pchote:voxels-to-mod-code branch Oct 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment