IO: New C++ PLY importer/exporter #104404
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#104404
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "super_jo_nathan/blender-PLY-project:D16792"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
NOTE: This patch has been worked on by Nathan Rozendaal, @Arjan van Diest (McDoodly) , @Lilith Houtjes (Stroopwafel) , @bas hendriks (bashendriks10) , @Thomas Feijen (ThomasF) and @Yoran Huzen (Kwonunn)
This is a patch for design task #101294 . This has as goal to implement a new and improved PLY importer/exporter in C++.
Implemented functionality
Importer
Exporter
Bug fixes and improvements
Vertex color changes
There are some differences in the way this importer/exporter handle vertex colors
Because the PLY spec does not specify a color space we left the conversion as simple as possible. Adjustments can be made in the future according to user feedback.
Unit test files
Are included in this PR
Performance
This has been tested with a mesh with ~400,000 verts | ~750,000 faces | with vertex colors
@ -0,0 +11,4 @@
#include "ply_export.hh"
#include "ply_import.hh"
void PLY_export(bContext *C, const struct PLYExportParams *export_params)
const struct PLYExportParams
->const PLYExportParams
More similar comments to previous reviews inline. We're getting there though!
@ -0,0 +9,4 @@
namespace blender::io::ply {
void write_vertices(std::unique_ptr<FileBuffer> &buffer, std::unique_ptr<PlyData> &plyData)
void write_vertices(std::unique_ptr<FileBuffer> &buffer, std::unique_ptr<PlyData> &plyData)
->
void write_vertices(FileBuffer &buffer, const PlyData &ply_data)
Not sure if the const is possible, but the function shouldn't need to know that the data is stored in unique pointers. Also, not a big deal, but if you're feeling motivated by consistency, the standard is
snake_case
for variable names.@ -0,0 +40,4 @@
BMeshFromMeshParams bm_convert_params{};
bm_convert_params.calc_face_normal = true;
bm_convert_params.calc_vert_normal = true;
bm_convert_params.add_key_index = false;
false
is the default forBMeshFromMeshParams
, there should be no need to specify that manually@ -0,0 +115,4 @@
const float2 *uv_map = static_cast<const float2 *>(
CustomData_get_layer(&mesh->ldata, CD_PROP_FLOAT2));
blender::Map<UV_vertex_key, int> vertex_map = generate_vertex_map(mesh, uv_map, export_params);
Specifying the
blender::
namespace here isn't necessary@ -0,0 +123,4 @@
/* Load faces into plyData. */
int loop_offset = 0;
Span<MLoop> loops = mesh->loops();
for (auto &&poly : mesh->polys()) {
for (auto &&poly : mesh->polys()) {
->
for (const MPoly &poly : mesh->polys()) {
Don't use auto for simple cases like this
@ -0,0 +185,4 @@
const StringRef name = mesh->active_color_attribute;
if (!name.is_empty()) {
const bke::AttributeAccessor attributes = mesh->attributes();
const VArray<ColorGeometry4f> colorAttribute =
colorAttribute
->color_attribute
@ -0,0 +70,4 @@
/* In binary, there is no end to a vertex. */
}
void write_face(char size, Vector<uint32_t> const &vertex_indices) override
Still seems like this could be much simpler. With two
write_bytes
calls, one forsize
and one for thevertex_indices
argument (which should also be a span in this case, there would be no need to copy the data to a localVector
first.Hi, I'm working on fixing this but I'm running into some trouble.
The issue is that I'm receiving the vertex_indices as a
Span<uint32_t>
but I want to write them asSpan<char>
. Reinterpret cast isn't letting me do that because "cast to typeblender::Span<char>
is not allowed".Is there a neat way of turning the
Span<uint32_t>
into aSpan<char>
without a loop?With the fix to another issue I've found a way to resolve this.
Pass a
Span
rather than a const pointer to an array:const Span<uint32_t> vertex_indices
@ -0,0 +188,4 @@
return (int)(it - header->properties[0].begin());
}
Vector<std::string> explode(const StringRef &str, const char &ch)
StringRef
is small and should usually be passed by value, otherwise it's a pointer to a pointer in the end.@ -0,0 +6,4 @@
#pragma once
#include "BKE_mesh.h"
Unused includes?
@ -0,0 +6,4 @@
#pragma once
#include "BKE_mesh.h"
Unused includes?
@ -0,0 +25,4 @@
CustomData_add_layer_named(
&mesh->vdata, CD_PROP_FLOAT3, CD_CONSTRUCT, nullptr, mesh->totvert, "position");
MutableSpan<float3> verts = mesh->vert_positions_for_write();
verts.copy_from(data.vertices);
How about simply
mesh->vert_positions_for_write().copy_from(data.vertices);
?@ -0,0 +83,4 @@
/* Uvmap */
if (!data.UV_coordinates.is_empty()) {
bke::SpanAttributeWriter<float2> Uv = attributes.lookup_or_add_for_write_only_span<float2>(
Uv
looks like a type, how aboutuv_map
instead?@ -0,0 +105,4 @@
/* Below code is necessary to access vertex normals within Blender.
* Until Blender supports custom vertex normals, this is a workaround. */
bke::SpanAttributeWriter<float3> normals;
Looking at this again, since the custom normals are set already, I don't think creating an attribute with normals should be included in this patch. We're planning on adding nodes to access custom normals to geometry nodes in 3.6, so this workaround wouldn't even really be necessary anyway.
@ -0,0 +117,4 @@
}
}
normals.finish();
BKE_mesh_set_custom_normals_from_verts(mesh, vertex_normals);
It looks to me like the temporary
vertex_normals
array isn't necessary.@ -0,0 +20,4 @@
Vector<float4> vertex_colors;
Vector<std::pair<int, int>> edges;
Vector<float3> edge_colors;
Vector<Vector<uint32_t>> faces;
Unless these faces are resized after they're added, it would be more efficient and clear to use
Vector<Array<uint32_t>>
facesApplied: D16792 new C++ based PLY importer/exporterto IO: New C++ PLY importer/exporterThanks for the update.
I'm still finding relatively basic cleanup stuff. But the impression I've gotten from reading the code a few times is that the overall logic is reasonable. I still hope that Aras has the time to look at this though, as someone more recently involved in this area.
@ -0,0 +97,4 @@
BKE_object_get_pre_modified_mesh(&export_object_eval_);
bool force_triangulation = false;
for (auto &&poly : mesh->polys()) {
auto &&poly
->const MPoly &poly
@ -0,0 +174,4 @@
const Span<float3> vert_normals = mesh->vertex_normals();
for (int i = 0; i < vertex_map.size(); i++) {
mul_m3_v3(world_and_axes_normal_transform_,
(float3)vert_normals[mesh_vertex_index_LUT[i]]);
This
(float3)
cast doesn't seem necessary@ -0,0 +56,4 @@
{
write_fstring("{}", int(count));
for (auto &&v : vertex_indices) {
Feel like I'm writing the same comment a few times now..
auto
is used sparingly in Blender source. In this case it's preferred to be explicit about the type, likefor (const uint32_t : ...)
. This helps the reader, since the type name is another clue about what's going on, and it's consistent with other code in Blender.@ -0,0 +74,4 @@
{
write_bytes(Span<char>({size}));
Span<char> dataSpan(reinterpret_cast<const char *>(vertex_indices.data()),
Span
has acast()
method that can be used here, likewrite_bytes(vertex_indices.cast<char>())
.@ -0,0 +90,4 @@
PlyHeader header;
while (true) { // We break when end_header is encountered.
Comment style
@ -0,0 +141,4 @@
else if ((words[0][0] >= '0' && words[0][0] <= '9') || words[0][0] == '-' || line.empty() ||
infile.eof()) {
/* A value was found before we broke out of the loop. No end_header. */
fprintf(stderr, "PLY Importer: failed to read file. No end_header.\n");
Not sure it's necessary to print the error when a report is already being used, especially if the information is exactly the same.
@ -0,0 +12,4 @@
namespace blender::io::ply {
std::unique_ptr<PlyData> import_ply_binary(std::ifstream &file, const PlyHeader *header)
{
std::unique_ptr<PlyData> data = std::make_unique<PlyData>();
The data inside unique pointers can be created directly in the constructor:
return std::make_unique<PlyData>(load_ply_binary(file, header));
@ -0,0 +34,4 @@
template<typename T> T swap_bytes(T input)
{
/* In big endian, the most-significant byte is first.
I'm a little skeptical about seeing this endian swapping implemented just for the ply importer, since it's a fairly generic thing that Blender already does.
This function itself seems fine, since it's templated which we don't do elsewhere. However, I'd suggest implementing each size case with the functions from
BLI_endian_switch.h
, so the logic isn't duplicated.Two other suggestions:
if
withif constexpr
reinterpret_cast
instead of C-style casts.@ -0,0 +44,4 @@
mesh->totpoly = int(data.faces.size());
mesh->totloop = 0;
for (int i = 0; i < data.faces.size(); i++) {
/* Add number of edges to the amount of edges. */
This comment probably means "loops" or "corners" rather than edges.
@ -0,0 +87,4 @@
int counter = 0;
for (int i = 0; i < data.faces.size(); i++) {
for (int j = 0; j < data.faces[i].size(); j++) {
copy_v2_v2(uv_map.span[counter], data.UV_coordinates[data.faces[i][j]]);
Should be possible to just use assignment rather than
copy_v2_v2
:uv_map.span[counter] = ...
@ -0,0 +13,4 @@
/**
* Converts the PlyData datastructure to a mesh.
* \param data The PLY data.
\param data
->\param data:
@ -0,0 +17,4 @@
* Reads a line in the ply file in a line-ending safe manner. All different line endings are
* supported. This also supports a mix of different line endings in the same file. CR (\\r), LF
* (\\n), CR/LF (\\r\\n), LF/CR (\\n\\r).
* @param file The file stream.
Convention in Blender is to use
\param file:
instead of@param file
@ -0,0 +1,478 @@
#include "testing/testing.h"
Missing license header
@ -0,0 +2,4 @@
#include "tests/blendfile_loading_base_test.h"
#include "BKE_blender_version.h"
#include "BKE_curve.h"
More unused includes (curve, material, more?)
@ -0,0 +1,261 @@
#include "testing/testing.h"
Missing license header
@ -0,0 +2,4 @@
#include "tests/blendfile_loading_base_test.h"
#include "BKE_attribute.hh"
#include "BKE_curve.h"
At least a few unused includes here (I'm noticing the curve ones right now, but there are probably others)
Overall very nice!
The comments that would be nice to fix before shipping this are std::ifstream usage (will fail on Windows with non-ASCII paths), and a question wrt vertex colors possibly needing sRGB conversion.
Other comments I've added are super minor and can be addressed in some later cleanup. I see a bunch of optimization opportunities as well (mostly on ascii importer side in string operations), but these can be done later too.
@ -0,0 +88,4 @@
uiItemR(sub, imfptr, "export_selected_objects", 0, IFACE_("Selected Only"), ICON_NONE);
uiItemR(sub, imfptr, "global_scale", 0, NULL, ICON_NONE);
row = uiLayoutRow(box, false);
Minor:
row
is not needed for laying this out. I assume it came initially from OBJ code back when it was using axis-choices-as-inline-buttons style. It should be enough to just do:and then
*row
variable above is not needed either.@ -0,0 +134,4 @@
}
/* Both forward and up axes cannot be along the same direction. */
static void forward_axis_update(struct Main *UNUSED(main),
Minor for future cleanup:
forward_axis_update
andup_axis_update
are exactly the same between OBJ and PLY code now. Might be good idea to move it into some shared place at some point (and while at it, maybe look whether Collada could share it).Done in
08db6bf215
@ -0,0 +49,4 @@
buffer = std::make_unique<FileBufferBinary>(export_params.filepath);
}
/* Generate and write header. */
Minor: these comments don't add much, it's very clear from the code flow what for example
write_header
does.@ -0,0 +50,4 @@
return temp_mesh;
}
void set_world_axes_transform(Object *object, const eIOAxis forward, const eIOAxis up)
Minor for future cleanup: this is the same code as OBJ set_world_axes_transform (and possibly others?). Would be nice to share.
@ -0,0 +20,4 @@
/* SEP macro from BLI path utils clashes with SEP symbol in fmt headers. */
#undef SEP
#define FMT_HEADER_ONLY
#include <fmt/format.h>
Minor: feels like
fmt/format.h
things are already included viaply_file_buffer.hh
above; should be no need to repeat them here. And fmt functionality is not directly used inFileBufferAscii
class anyway.@ -0,0 +22,4 @@
#undef SEP
#define FMT_HEADER_ONLY
#include <bitset>
#include <fmt/format.h>
Similar here:
fmt/format.h
includes feel like they are not much needed.@ -0,0 +86,4 @@
{
std::string line;
std::ifstream infile(import_params.filepath, std::ios::binary);
std::ifstream
will fail on Windows when file path contains non-English characters. Useblender::fstream
wrapper fromBLI_fileops.hh
instead.@ -0,0 +175,4 @@
normal.z = read<float>(file, isBigEndian);
}
else if (name == "red") {
color.x = read<uint8_t>(file, isBigEndian) / 255.0f;
What colorspace are PLY vertex colors defined in? Since they seem to be 8 bits per component, I would assume they are sRGB (linear colors would not have enough precision at just 8 bits). My impression is that Blender colors are in linear color space, when they are stored as floats. So the reading code here (and likewise, exporting code elsewhere) might need a sRGB<->Linear conversion.
The PLY file format was made before the release of sRGB (1994 vs 1999). So I doubt it was made with that in mind. But the colours in the specification are defined as an integer range from 0-255 (so 8-bit colour per channel) which is why we read it as a
uint8_t
.The PLY spec says nothing about colour space, and we have no way of knowing how other programmes interact with PLY files and if they use sRGB or not. But we've decided on assuming sRGB now and will make the conversion between the two.
I spotted the function
srgb_to_linearrgb_v3_v3(linear, srgb);
in the OBJ importer, and from the source alsosrgb_to_linearrgb_v4
. Since we assume an alpha channel we will use that function to convert in the importer, and its opposite for the exporter.On further inspection and importing a test file, we have found that using the sRGB -> linear conversion gives too dark colours compared to the original (see pictures)
Original file/website
With sRGB->Linear conversion:
Original implementation:
Interesting. So it might be that various people just write linear colors in there, without realizing that at 8 bits per channel there's severe banding in the darks :) Or alternatively, the importer/exporter might need a choice between sRGB or Linear vertex colors, very similar to how FBX importer/exporter in Blender has (FBX is also in the "vertex colors are in unspecified color space, and various apps treat it differently").
I've implemented option for vertex colors Linear vs sRGB color space in
a30abe9c2e
. Defaulted to sRGB to match the current Python addon behavior.My part of feedback was addressed, thanks!
Scrolled through the diff and only noticed a couple more things. They're small, so I'll accept the patch. Though
main
needs to be merged into the branch now, a couple things there. Then I think this is good to go!@ -0,0 +218,4 @@
DEG_OBJECT_ITER_END;
}
blender::Map<UV_vertex_key, int> generate_vertex_map(const Mesh *mesh,
blender::Map
->Map
@ -0,0 +223,4 @@
const PLYExportParams &export_params)
{
blender::Map<UV_vertex_key, int> vertex_map;
blender::Map
->Map
@ -0,0 +52,4 @@
{
write_bytes(Span<char>({size}));
Span<char> dataSpan(reinterpret_cast<const char *>(vertex_indices.data()),
Once the argument is a
Span
, usevertex_indices.cast<char>()
Generally, I think it's ok from my side as well. Most things from my phab review have been taken care of and a test of some .ply files from older bugs seem ok.
I do want to again note the vertex color differences between Old and New that should be documented once this lands so we can point users to it if they have questions.
I updated the description to be up to date with the changes made and the feedback given. As far as I can see we should be ready for a merge.
Merged (squashed) in
43e9c90061
🎉Pull request closed