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

Vox importer rewrite. New features and fixes #49

Merged
merged 11 commits into from
Jul 27, 2021

Conversation

ElectronicBlueberry
Copy link
Contributor

A rewrite on the vox file importer, with minor additions, tweaks and fixes to the other importers, and voxel objects.
Importer partially based on: https://github.com/jpaver/opengametools/blob/master/src/ogt_vox.h

The main new feature is the ability to fully import Magica Voxel files as scenes, while keeping all groups and objects separate. This allows for a faster workflow with Magica Voxel, skipping previously required import steps, when importing complex vox files.

This PR also fixes a missing sRGB flag on the default material, wrong forward orientation for imported voxel models, and incompatibility with newer vox files. It also introduces a new option for a per-model voxel size, and allows for more control over centering on import.

sRGB Color, and Vox compatibility example:

Old New
ColorAndFacingOld ColorAndFacingNew
CharOld CharNew

As seen in the pictures, the forward direction is also changed. Previously the vox importer kept Magica Voxels x axis direction, but as a result models faced backwards by default. The new importer flips the x-axis, keeping the forward direction from Magica Voxel aligned to Godots -z forward direction.

New import options, and importer:

import options new importer imported vox file, with new importer
OptionsNew SceneImporter ImportedScene

The "Voxel Size" is now no longer global, and can be set per object. This works with voxel meshes, as well as voxel objects, and the voxel editor. A new editor option has been added, to re-scale the voxels while editing.

The new Magica Voxel Scene importer imports a vox file as seperate objects, keeping all included groups, shapes, and their respective transforms and names. Shapes are imported as Mesh Instances, and Groups as Spatials. This feature was the key reason for starting this PR, as it drastically speeds up the workflow in my current project.

Note: In it's current form this PR is a WIP. While all features I set out to add are present, it has not yet been tested thoroughly enough to be considered production ready. I would appreciate testing and feedback.

Copy link
Owner

@ClarkThyLord ClarkThyLord left a comment

Choose a reason for hiding this comment

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

First of, thanks, this is truly amazing work, as you said this is definitely something much needed to have a faster and more fluid workflow between MagicaVoxel and Godot.

Now, I've done a surface review of the code and everything seems to look good. Only needs a bit cleaning up and formatting, but that can be done quickly so no worries. I'll take a closer look and test it more thoroughly this weekend with some practical cases to make sure everything is ready to merge. 👍

I also see that besides the MagicaVoxel importer rework, you've also taken the liberty of having the voxel_size be defined per VoxelObject. This is something I had originally contemplated when implementing the plugin, that is: having the voxel's size be static for all VoxelObjects or have every VoxelObject define their own voxel size. I ended up making it static since I couldn't think of many cases where having each VoxelObject define their own voxel size would be useful, since you could always scale the VoxelObject, and therefore its voxels, as you could with any other MeshInstance. However, if you feel that there is a need for this change then I'm more than happy to accept these changes.

Thanks again, amazing work, looking forward to trying it out! 🙌

addons/voxel-core/voxel-core.gd Outdated Show resolved Hide resolved
addons/voxel-core/classes/readers/vox.gd Outdated Show resolved Hide resolved
addons/voxel-core/engine/importers/meshes.gd Outdated Show resolved Hide resolved
addons/voxel-core/engine/importers/meshes.gd Outdated Show resolved Hide resolved
addons/voxel-core/engine/importers/vox_scenes.gd Outdated Show resolved Hide resolved
addons/voxel-core/classes/readers/vox.gd Outdated Show resolved Hide resolved
@ClarkThyLord ClarkThyLord added the enhancement New feature or request label Jul 20, 2021
- fixed typos in comments
- removed debug prints
- fixed rotation import
- added new importer base class
- renamed importers for consistency
@ElectronicBlueberry
Copy link
Contributor Author

ElectronicBlueberry commented Jul 21, 2021

Thanks for the quick response and surface review!

I resolved the requested changes, and added a importer base class, removing the duplicate code.
For the centering to work consistently across importers, I changed the voxel_object centering function to assume that 0 represents the lowest relative position in the mesh, and 1 the highest. Since this might be a breaking change for some projects, I could alternatively change the scene importer to swap the relative coordinates around.

As for the per object voxel size: It's definitely not a must have feature, but can be helpful in some cases, as scaling the root for a voxel scene can become confusing when working with local coordinates in code.

Keeping new code more consistent with the rest of code base
Clark was here :V
Editing button needs to listen for "toggled" events to properly toggle between modes
Clark was here :V
Not needed as "VOXEL_SCENE" is not a new Voxel-Core class
Copy link
Owner

@ClarkThyLord ClarkThyLord left a comment

Choose a reason for hiding this comment

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

Okay, I've taken a deeper look at everything, at it looks great 🙌
I was able to make use of everything as expected, and only found a few small issues.
The reworked Vox importer looks and works great, I've only seen a few corner cases where it fails.

MagicaVoxel Godot
Screenshot 2021-07-25 104816 Screenshot 2021-07-25 104612
This is the model from within MagicaVoxel, as you can see it's two objects: One is the Knight with no rotation, and the other is the Axe with some z-rotation. The model is saved as a project, making it a extended vox file. This is that project imported into Godot as a VoxelObject, as you can see everything looks good, however the Axe is set one more voxel to the right, which causes it to overwrite the Knight's right hand.

I found this issue while testing the importer, something similar happens when you import the same project as a MagicaVoxelScene.

MagicaVoxel Godot
Screenshot 2021-07-25 212352 Screenshot 2021-07-25 212332
Side view of the same project from within MagicaVoxel. This is that project imported into Godot as a MagicaVoxelScene, the Axe is set one more voxel to the right and forward.

ZIP for this MagicaVoxel scene: chr_knight_and_axe.project.zip

Not sure if you know what might be causing this? I'm thinking it has to do with the way MagicaVoxel's positioning work, or maybe the center of these "objects" are not being calculated correctly somehow 🤔

Other then that everything looks great, so I'd be willing to merge right now, unless you have any objections, thanks again for this great work! 🙌

I've also gone ahead and done some refactoring and formatting to the code to make it consistent with the rest of the code base 👍

@ElectronicBlueberry
Copy link
Contributor Author

ElectronicBlueberry commented Jul 26, 2021

Good catch on that edge case! It was caused by the x axis being rounded down with floor(), while it should be rounded up during import, because it is flipped from MagicaVoxel. This also caused the origin to shift on the knight model, which is now also fixed.

No objections to a merge from my side, I used the importer the past couple of days, and it seems stable.

Copy link
Owner

@ClarkThyLord ClarkThyLord left a comment

Choose a reason for hiding this comment

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

Nice, just tested the changes and it looks good 👌

I'll probably rework the Readers later to make them more consistent, because right now what they return isn't really well defined. I also need to redesign them to be able to accommodate for different structures and needs in the future, like that of "voxel scenes".

But that's a different topic for another time, for now I'll move forward with the merge, thanks again for sharing the great work! 🙌

@ClarkThyLord ClarkThyLord merged commit f8469ed into ClarkThyLord:master Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Import multiple objects from complex MagicaVoxel File
2 participants