-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[Impeller Scene] Render imported meshes #38097
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very exciting! Can't wait to tinker on this. LGTM with some nits.
|
||
namespace impeller { | ||
namespace scene { | ||
|
||
class CuboidGeometry; | ||
class VertexBufferGeometry; | ||
|
||
class Geometry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is unrelated to this patch so we can fix this later, but, Geometry
should have a virtual destructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, nice catch. Done.
indices->type = fb::IndicesType::k32Bit; | ||
break; | ||
default: | ||
std::cerr << "Mesh primitive has unsupported index type " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a validation log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed types and floats are certainly invalid. But unsigned byte is technically valid per the spec. I need to go through and add conversion paths for some of the weirder cases like this.
VertexBuffer GetVertexBuffer(Allocator& allocator) const override; | ||
|
||
private: | ||
VertexBuffer vertex_buffer_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and elsewhere, DISALLOW_COPY_AND_ASSIGN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
public: | ||
void SetVertexBuffer(VertexBuffer vertex_buffer); | ||
|
||
VertexBuffer GetVertexBuffer(Allocator& allocator) const override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: For later if needed. Let's stick to the style guide and note the superclass that defines this overriden method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
table Indices { | ||
data: [ubyte]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these arrays already encode their size. So the count would be redundant right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The count isn't for the number of bytes, but for the number of indices. However, I think we should remove this field and just compute this value inline based on the type, since we shouldn't rely on any of the data being trustworthy. Will do so in a follow-up.
…116633) * 5429243d3 [Impeller Scene] Render imported meshes (flutter/engine#38097) * 67254d6e4 Use announce function in live region (flutter/engine#38084)
…lutter#116633) * 5429243d3 [Impeller Scene] Render imported meshes (flutter/engine#38097) * 67254d6e4 Use announce function in live region (flutter/engine#38084)
…lutter#116633) * 5429243d3 [Impeller Scene] Render imported meshes (flutter/engine#38097) * 67254d6e4 Use announce function in live region (flutter/engine#38084)
Humble beginnings!