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

add cesium ext_mesh_features #199

Merged
merged 16 commits into from
Nov 22, 2023

Conversation

bertt
Copy link
Contributor

@bertt bertt commented Nov 1, 2023

@bertt bertt marked this pull request as draft November 1, 2023 10:11
@vpenades
Copy link
Owner

vpenades commented Nov 1, 2023

I can't find the code that generates the ext.MeshFeaturesMesh.g.cs file.

Ideally, the PR should include the Build\ SharpGLTF.CodeGen code, so the generator and the generated code can be tracked to the same PR

It is fine to modify the codegen project to either:

  • call the method that clones the repository to get the extension from there.
  • simply include the schemas as content files (maybe in a LocalSchemas\ directory

@bertt
Copy link
Contributor Author

bertt commented Nov 8, 2023

Amended Code generation for FeatureIdTexture.
@vpenades
Copy link
Owner

I've fixed the code generator, the field access was in the wrong method, and there's no good way of defining a default value, so I completely removed it (which I think it's the best choice)

I also changed the generated class names to something less generic as "FeatureID"

It's odd to me that these cesium extensions are "EXT" extensions, but I think that if they originated from Cesium they should be called something else, I don't know.

Anyway, I also dislike these alternate names, they're provisional until we settle on a better naming convention.

@bertt bertt marked this pull request as ready for review November 14, 2023 11:59
@@ -4,7 +4,7 @@
namespace SharpGLTF.Schema2
{
[System.Diagnostics.DebuggerDisplay("LogicalTexture[{_LogicalTextureIndex}]")]
internal partial class TextureInfo
public partial class TextureInfo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must be public otherwise generated code MeshExtMeshFeatureIDTexture : TextureInfo does not compile (error: 'TextureInfo is inaccessible due to it's protection level')

Copy link
Owner

@vpenades vpenades Nov 14, 2023

Choose a reason for hiding this comment

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

Must be public

I agree, as more extensions are created outside the core repository, there'll be a demand for exposing some internals.

On the current extension:

Eventually, these classes will also need constructors, specialised collections and probably some validation.

For example, I think "attribute" is actually an index to an Accessor, so it will probably require the same wiring used in cesium_outline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its little different, if attribute=0 then there must be a custom vertex attribute with name _FEATURE_ID_0, we can check on that

Copy link
Owner

Choose a reason for hiding this comment

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

To tell you the truth, I don't fully understand some of the details of this extension, specially the bit-based thing of the feature Id... and I understand the general concepts of the extension, but I haven't fully grasped all the details, so I am relying on you to flesh out this extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vpenades vpenades merged commit fbb0d77 into vpenades:master Nov 22, 2023
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.

2 participants