IO: New C++ PLY importer/exporter #104404

Closed
Nathan Rozendaal wants to merge 10 commits from super_jo_nathan/blender-PLY-project:D16792 into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
First-time contributor

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

  • Vertices
  • Faces
  • Edges
  • Vertex colors
  • Preliminary support for custom vertex normals
  • UV coordinates
  • ASCII
  • binary little endian
  • binary big endian

Exporter

  • Vertices
  • Faces
  • Edges
  • Vertex colors
  • Vertex normals
  • UV coordinates
  • Triangulation
  • ASCII
  • binary little endian

Bug fixes and improvements

  • Importing point clouds with vertex colors now works
  • Importing PLY files with non standard line endings
  • Exporting multiple objects (previous exporter didn't take the vertex indices into account)
  • Notable differences
  • The importer has the option to merge vertices, this is useful because when importing PLY files with UV maps the mesh is split along UV seams.
  • The exporter supports exporting loose edges and vertices along with UV map data.

Vertex color changes

There are some differences in the way this importer/exporter handle vertex colors

Old New
Attribute type: Face Corner -> Byte Color Vertex -> Color
Color space: linear? Simple conversion from uchar to float
Is the attribute active?: Yes, enabled for Render No, not enabled for Render (can address this later after this lands)

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

image

image

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 * Vertices * Faces * Edges * Vertex colors * Preliminary support for custom vertex normals * UV coordinates * ASCII * binary little endian * binary big endian ### Exporter * Vertices * Faces * Edges * Vertex colors * Vertex normals * UV coordinates * Triangulation * ASCII * binary little endian ## Bug fixes and improvements * Importing point clouds with vertex colors now works * Importing PLY files with non standard line endings * Exporting multiple objects (previous exporter didn't take the vertex indices into account) * Notable differences * The importer has the option to merge vertices, this is useful because when importing PLY files with UV maps the mesh is split along UV seams. * The exporter supports exporting loose edges and vertices along with UV map data. ## Vertex color changes There are some differences in the way this importer/exporter handle vertex colors | | Old | New | | -------- | -------- | -------- | | Attribute type: | Face Corner -> Byte Color | Vertex -> Color | | Color space: | linear? | Simple conversion from uchar to float| | Is the attribute active?: | Yes, enabled for Render | No, not enabled for Render (can address this later after this lands)| 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 ![image](/attachments/aa0d24c6-504b-4064-a841-a94234c7a155) ![image](/attachments/1d13b558-88af-4596-9166-822dabb4fa6d)
Nathan Rozendaal added 1 commit 2023-02-07 16:34:13 +01:00
Nathan Rozendaal requested review from Hans Goudey 2023-02-07 16:35:07 +01:00
Hans Goudey refused to review 2023-02-07 17:31:45 +01:00
Hans Goudey requested review from Aras Pranckevicius 2023-02-07 17:31:45 +01:00
Hans Goudey requested review from Hans Goudey 2023-02-07 17:31:59 +01:00
Hans Goudey reviewed 2023-02-07 17:35:33 +01:00
@ -0,0 +11,4 @@
#include "ply_export.hh"
#include "ply_import.hh"
void PLY_export(bContext *C, const struct PLYExportParams *export_params)
Member

const struct PLYExportParams -> const PLYExportParams

`const struct PLYExportParams` -> `const PLYExportParams`
super_jo_nathan marked this conversation as resolved
Hans Goudey requested changes 2023-02-07 18:13:18 +01:00
Hans Goudey left a comment
Member

More similar comments to previous reviews inline. We're getting there though!

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)
Member

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.

`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.
super_jo_nathan marked this conversation as resolved
@ -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;
Member

false is the default for BMeshFromMeshParams, there should be no need to specify that manually

`false` is the default for `BMeshFromMeshParams`, there should be no need to specify that manually
super_jo_nathan marked this conversation as resolved
@ -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);
Member

Specifying the blender:: namespace here isn't necessary

Specifying the `blender::` namespace here isn't necessary
super_jo_nathan marked this conversation as resolved
@ -0,0 +123,4 @@
/* Load faces into plyData. */
int loop_offset = 0;
Span<MLoop> loops = mesh->loops();
for (auto &&poly : mesh->polys()) {
Member

for (auto &&poly : mesh->polys()) {
->
for (const MPoly &poly : mesh->polys()) {

Don't use auto for simple cases like this

`for (auto &&poly : mesh->polys()) {` -> `for (const MPoly &poly : mesh->polys()) {` Don't use auto for simple cases like this
super_jo_nathan marked this conversation as resolved
@ -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 =
Member

colorAttribute -> color_attribute

`colorAttribute` -> `color_attribute`
super_jo_nathan marked this conversation as resolved
@ -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
Member

Still seems like this could be much simpler. With two write_bytes calls, one for size and one for the vertex_indices argument (which should also be a span in this case, there would be no need to copy the data to a local Vector first.

Still seems like this could be much simpler. With two `write_bytes` calls, one for `size` and one for the `vertex_indices` argument (which should also be a span in this case, there would be no need to copy the data to a local `Vector` first.
First-time contributor

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 as Span<char>. Reinterpret cast isn't letting me do that because "cast to type blender::Span<char> is not allowed".

Is there a neat way of turning the Span<uint32_t> into a Span<char> without a loop?

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 as `Span<char>`. Reinterpret cast isn't letting me do that because "cast to type `blender::Span<char>` is not allowed". Is there a neat way of turning the `Span<uint32_t>` into a `Span<char>` without a loop?
First-time contributor

With the fix to another issue I've found a way to resolve this.

With the fix to another issue I've found a way to resolve this.
Member

Pass a Span rather than a const pointer to an array:

const Span<uint32_t> vertex_indices

Pass a `Span` rather than a const pointer to an array: `const Span<uint32_t> vertex_indices`
super_jo_nathan marked this conversation as resolved
@ -0,0 +188,4 @@
return (int)(it - header->properties[0].begin());
}
Vector<std::string> explode(const StringRef &str, const char &ch)
Member

StringRef is small and should usually be passed by value, otherwise it's a pointer to a pointer in the end.

`StringRef` is small and should usually be passed by value, otherwise it's a pointer to a pointer in the end.
super_jo_nathan marked this conversation as resolved
@ -0,0 +6,4 @@
#pragma once
#include "BKE_mesh.h"
Member

Unused includes?

Unused includes?
super_jo_nathan marked this conversation as resolved
@ -0,0 +6,4 @@
#pragma once
#include "BKE_mesh.h"
Member

Unused includes?

Unused includes?
super_jo_nathan marked this conversation as resolved
@ -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);
Member

How about simply mesh->vert_positions_for_write().copy_from(data.vertices);?

How about simply `mesh->vert_positions_for_write().copy_from(data.vertices);`?
super_jo_nathan marked this conversation as resolved
@ -0,0 +83,4 @@
/* Uvmap */
if (!data.UV_coordinates.is_empty()) {
bke::SpanAttributeWriter<float2> Uv = attributes.lookup_or_add_for_write_only_span<float2>(
Member

Uv looks like a type, how about uv_map instead?

`Uv` looks like a type, how about `uv_map` instead?
super_jo_nathan marked this conversation as resolved
@ -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;
Member

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.

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.
super_jo_nathan marked this conversation as resolved
@ -0,0 +117,4 @@
}
}
normals.finish();
BKE_mesh_set_custom_normals_from_verts(mesh, vertex_normals);
Member

It looks to me like the temporary vertex_normals array isn't necessary.

It looks to me like the temporary `vertex_normals` array isn't necessary.
super_jo_nathan marked this conversation as resolved
@ -0,0 +20,4 @@
Vector<float4> vertex_colors;
Vector<std::pair<int, int>> edges;
Vector<float3> edge_colors;
Vector<Vector<uint32_t>> faces;
Member

Unless these faces are resized after they're added, it would be more efficient and clear to use Vector<Array<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>>` faces
super_jo_nathan marked this conversation as resolved
Hans Goudey changed title from Applied: D16792 new C++ based PLY importer/exporter to IO: New C++ PLY importer/exporter 2023-02-07 22:03:59 +01:00
Brecht Van Lommel added this to the Pipeline, Assets & IO project 2023-02-13 09:19:37 +01:00
Nathan Rozendaal added 2 commits 2023-02-16 15:11:30 +01:00
Hans Goudey requested changes 2023-02-16 15:55:15 +01:00
Hans Goudey left a comment
Member

Thanks 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.

Thanks 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()) {
Member

auto &&poly -> const MPoly &poly

`auto &&poly` -> `const MPoly &poly`
super_jo_nathan marked this conversation as resolved
@ -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]]);
Member

This (float3) cast doesn't seem necessary

This `(float3)` cast doesn't seem necessary
super_jo_nathan marked this conversation as resolved
@ -0,0 +56,4 @@
{
write_fstring("{}", int(count));
for (auto &&v : vertex_indices) {
Member

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, like for (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.

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, like `for (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.
super_jo_nathan marked this conversation as resolved
@ -0,0 +74,4 @@
{
write_bytes(Span<char>({size}));
Span<char> dataSpan(reinterpret_cast<const char *>(vertex_indices.data()),
Member

Span has a cast() method that can be used here, like write_bytes(vertex_indices.cast<char>()).

`Span` has a `cast()` method that can be used here, like `write_bytes(vertex_indices.cast<char>())`.
super_jo_nathan marked this conversation as resolved
@ -0,0 +90,4 @@
PlyHeader header;
while (true) { // We break when end_header is encountered.
Member

Comment style

Comment style
super_jo_nathan marked this conversation as resolved
@ -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");
Member

Not sure it's necessary to print the error when a report is already being used, especially if the information is exactly the same.

Not sure it's necessary to print the error when a report is already being used, especially if the information is exactly the same.
super_jo_nathan marked this conversation as resolved
@ -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>();
Member

The data inside unique pointers can be created directly in the constructor:

return std::make_unique<PlyData>(load_ply_binary(file, header));

The data inside unique pointers can be created directly in the constructor: `return std::make_unique<PlyData>(load_ply_binary(file, header));`
super_jo_nathan marked this conversation as resolved
@ -0,0 +34,4 @@
template<typename T> T swap_bytes(T input)
{
/* In big endian, the most-significant byte is first.
Member

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:

  • Replace if with if constexpr
  • Use reinterpret_cast instead of C-style casts.
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: - Replace `if` with `if constexpr` - Use `reinterpret_cast` instead of C-style casts.
super_jo_nathan marked this conversation as resolved
@ -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. */
Member

This comment probably means "loops" or "corners" rather than edges.

This comment probably means "loops" or "corners" rather than edges.
super_jo_nathan marked this conversation as resolved
@ -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]]);
Member

Should be possible to just use assignment rather than copy_v2_v2:

uv_map.span[counter] = ...

Should be possible to just use assignment rather than `copy_v2_v2`: `uv_map.span[counter] = ...`
super_jo_nathan marked this conversation as resolved
@ -0,0 +13,4 @@
/**
* Converts the PlyData datastructure to a mesh.
* \param data The PLY data.
Member

\param data -> \param data:

`\param data` -> `\param data:`
super_jo_nathan marked this conversation as resolved
@ -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.
Member

Convention in Blender is to use \param file: instead of @param file

Convention in Blender is to use `\param file:` instead of `@param file`
super_jo_nathan marked this conversation as resolved
@ -0,0 +1,478 @@
#include "testing/testing.h"
Member

Missing license header

Missing license header
super_jo_nathan marked this conversation as resolved
@ -0,0 +2,4 @@
#include "tests/blendfile_loading_base_test.h"
#include "BKE_blender_version.h"
#include "BKE_curve.h"
Member

More unused includes (curve, material, more?)

More unused includes (curve, material, more?)
super_jo_nathan marked this conversation as resolved
@ -0,0 +1,261 @@
#include "testing/testing.h"
Member

Missing license header

Missing license header
super_jo_nathan marked this conversation as resolved
@ -0,0 +2,4 @@
#include "tests/blendfile_loading_base_test.h"
#include "BKE_attribute.hh"
#include "BKE_curve.h"
Member

At least a few unused includes here (I'm noticing the curve ones right now, but there are probably others)

At least a few unused includes here (I'm noticing the curve ones right now, but there are probably others)
super_jo_nathan marked this conversation as resolved
Aras Pranckevicius requested changes 2023-02-17 22:15:17 +01:00
Aras Pranckevicius left a comment
Member

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.

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:

uiItemR(sub, imfptr, "forward_axis", 0, IFACE_("Forward Axis"), ICON_NONE);
uiItemR(sub, imfptr, "up_axis", 0, IFACE_("Up Axis"), ICON_NONE);

and then *row variable above is not needed either.

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: ``` uiItemR(sub, imfptr, "forward_axis", 0, IFACE_("Forward Axis"), ICON_NONE); uiItemR(sub, imfptr, "up_axis", 0, IFACE_("Up Axis"), ICON_NONE); ``` and then `*row` variable above is not needed either.
super_jo_nathan marked this conversation as resolved
@ -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 and up_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).

Minor for future cleanup: `forward_axis_update` and `up_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

Done in 08db6bf215ab
aras_p marked this conversation as resolved
@ -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.

Minor: these comments don't add much, it's very clear from the code flow what for example `write_header` does.
super_jo_nathan marked this conversation as resolved
@ -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.

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 via ply_file_buffer.hh above; should be no need to repeat them here. And fmt functionality is not directly used in FileBufferAscii class anyway.

Minor: feels like `fmt/format.h` things are already included via `ply_file_buffer.hh` above; should be no need to repeat them here. And fmt functionality is not directly used in `FileBufferAscii` class anyway.
super_jo_nathan marked this conversation as resolved
@ -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.

Similar here: `fmt/format.h` includes feel like they are not much needed.
super_jo_nathan marked this conversation as resolved
@ -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. Use blender::fstream wrapper from BLI_fileops.hh instead.

`std::ifstream` will fail on Windows when file path contains non-English characters. Use `blender::fstream` wrapper from `BLI_fileops.hh` instead.
super_jo_nathan marked this conversation as resolved
@ -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.

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.
First-time contributor

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 also srgb_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.

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 also `srgb_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.
First-time contributor

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:
With sRGB->Linear

Original implementation:
Without sRGB->Linear

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](https://www.artec3d.com/3d-models/doom-combat-scene) With sRGB->Linear conversion: ![With sRGB->Linear](https://cdn.discordapp.com/attachments/1015698649486999577/1076464344910544986/image.png) Original implementation: ![Without sRGB->Linear](https://cdn.discordapp.com/attachments/1015698649486999577/1076464979252879370/image.png)

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").

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.

I've implemented option for vertex colors Linear vs sRGB color space in a30abe9c2e79a01. Defaulted to sRGB to match the current Python addon behavior.
aras_p marked this conversation as resolved
Nathan Rozendaal added 2 commits 2023-02-27 19:01:46 +01:00
Nathan Rozendaal added 1 commit 2023-02-27 19:27:30 +01:00
Aras Pranckevicius approved these changes 2023-02-28 20:21:53 +01:00
Aras Pranckevicius left a comment
Member

My part of feedback was addressed, thanks!

My part of feedback was addressed, thanks!
Hans Goudey approved these changes 2023-03-01 00:09:22 +01:00
Hans Goudey left a comment
Member

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!

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,
Member

blender::Map -> Map

`blender::Map` -> `Map`
super_jo_nathan marked this conversation as resolved
@ -0,0 +223,4 @@
const PLYExportParams &export_params)
{
blender::Map<UV_vertex_key, int> vertex_map;
Member

blender::Map -> Map

`blender::Map` -> `Map`
super_jo_nathan marked this conversation as resolved
@ -0,0 +52,4 @@
{
write_bytes(Span<char>({size}));
Span<char> dataSpan(reinterpret_cast<const char *>(vertex_indices.data()),
Member

Once the argument is a Span, use vertex_indices.cast<char>()

Once the argument is a `Span`, use `vertex_indices.cast<char>()`
super_jo_nathan marked this conversation as resolved

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.

. Old New
Attribute type Face Corner -> Byte Color Vertex -> Color
Space linear? sRGB? Loads with a value of 0.5 whereas old loads value of 0.22
Is the attribute active? Yes, enabled for Render No, not enabled for Render (can address this later after this lands.
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. | . | Old | New | | --- | --- | --- | | Attribute type | Face Corner -> Byte Color | Vertex -> Color | | Space | linear? | sRGB? Loads with a value of 0.5 whereas old loads value of 0.22 | | Is the attribute active? | Yes, enabled for Render | No, not enabled for Render (can address this later after this lands. |
Author
First-time contributor

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.

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 🎉

Merged (squashed) in 43e9c90061fc1ebae 🎉
Bastien Montagne removed this from the Pipeline, Assets & IO project 2023-07-03 13:02:33 +02:00

Pull request closed

Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#104404
No description provided.