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

Improve documentation #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve documentation #33

wants to merge 1 commit into from

Conversation

mchorse
Copy link

@mchorse mchorse commented Feb 28, 2020

Thank you very much for such awesome voxel editing software! 😊

However, I had some troubles understanding the extension documentation, specifically rotation and how do assemble the scene graph so the objects would correctly show up on their places. After hours of fiddling with with, I managed to get it work.

It would be nice if the documentation could've been a little bit more specific on some details. A lot of people (like me) don't have the brightest mind, and so these additional details could be quite useful!

@Snkz
Copy link

Snkz commented May 18, 2020

I think the wording in the change could use alot of improvement. It should be considerably more concise and avoid using terminology that is normally used in other contexts (compliment for example)

@mchorse
Copy link
Author

mchorse commented May 18, 2020

@Snkz any concrete suggestions?

Copy link

@Snkz Snkz left a comment

Choose a reason for hiding this comment

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

These are the blocks i think could use work

@@ -77,6 +77,10 @@ M | | children chunks
-------------------------------------------------------------------------------
4 | int | numVoxels (N)
4 x N | int | (x, y, z, colorIndex) : 1 byte for each component

| Note: Z axis is vertical axis, so you might want to
Copy link

Choose a reason for hiding this comment

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

would remove this note

@@ -59,13 +63,28 @@ S : Shape Node
| |
S S

Note: in order to build transformations, you need to traverse the scene
Copy link

@Snkz Snkz May 18, 2020

Choose a reason for hiding this comment

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

How to maintain your parsed data shouldnt be part of the doc, would re write this note entirely to just mention that connection between shape nodes and transform nodes and the parenting structure.

@@ -40,6 +40,10 @@ bit | value
5 : 1 : the sign in the second row (0 : positive; 1 : negative)
6 : 1 : the sign in the third row (0 : positive; 1 : negative)

Note: the index of the non-zero entry of the third row is remaining
Copy link

Choose a reason for hiding this comment

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

Rewritten in clearer language

@@ -23,7 +23,7 @@ STRING : value
=================================
(c) ROTATION type

store a row-major rotation in the bits of a byte
store a row-major (XZY) rotation in the bits of a byte
Copy link

Choose a reason for hiding this comment

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

Is that right? Shouldn't it be "(XYZ)" ?

Copy link
Author

Choose a reason for hiding this comment

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

Is that right? Shouldn't it be "(XYZ)" ?

It's been a while since I've worked with MagicaVoxel, but if I remember correctly, this should be correct. At least from what I remember. If what not, this is the code that worked for me to read the correct rotation for my Minecraft mod.

Choose a reason for hiding this comment

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

The following transformation works as intended:

let vector = Vec3::new(
    x * matrix.m11 + y * matrix.m12 + z * matrix.m13,
    x * matrix.m21 + y * matrix.m22 + z * matrix.m23,
    x * matrix.m31 + y * matrix.m32 + z * matrix.m33,
);

This suggests an XYZ rotation matrix

Comment on lines +80 to +83

| Note: Z axis is vertical axis, so you might want to
| read them first and then assemble as (x, z, y) for Y-up
| coordinate system.

Choose a reason for hiding this comment

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

Suggested change
| Note: Z axis is vertical axis, so you might want to
| read them first and then assemble as (x, z, y) for Y-up
| coordinate system.
| Note: Coordinates are in a Right handed Z-up coordinate system

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.

4 participants