Fix #112068: Crash when loading converted curves #112280

Falk David · 2023-09-12T14:02:17+02:00

Falk David commented

2023-09-12 14:02:17 +02:00

In 2788fa915b the CurvesGeometry::blend_write and CurvesGeometry::blend_read functions were added. Unfortunately, the commit also altered the writing logic and introduced a bug where loading a file with a converted Curves object would crash. See #112068.

This PR fixes the issue by making sure that CustomData_blend_write_prepare is called before BKE_id_blend_write, which is the root cause of the crash that happens on load.

The CurvesGeometry::blend_write function is split into CurvesGeometry::blend_write_prepare and CurvesGeometry::blend_write.

In https://projects.blender.org/blender/blender/commit/2788fa915be6a7da0a0b0ad63626c095eef1e216 the `CurvesGeometry::blend_write` and `CurvesGeometry::blend_read` functions were added. Unfortunately, the commit also altered the writing logic and introduced a bug where loading a file with a converted `Curves` object would crash. See https://projects.blender.org/blender/blender/issues/112068. This PR fixes the issue by making sure that `CustomData_blend_write_prepare` is called before `BKE_id_blend_write`, which is the root cause of the crash that happens on load. The `CurvesGeometry::blend_write` function is split into `CurvesGeometry::blend_write_prepare` and `CurvesGeometry::blend_write`.

Falk David added 1 commit 2023-09-12 14:02:29 +02:00

c6db798942 Fix #112068 : Crash when loading converted curves

Falk David requested review from Hans Goudey 2023-09-12 14:03:32 +02:00

Falk David requested review from Bastien Montagne 2023-09-12 14:03:42 +02:00

Hans Goudey approved these changes 2023-09-12 14:13:07 +02:00

Bastien Montagne requested changes 2023-09-12 14:24:47 +02:00

Bastien Montagne left a comment

There should be a proper technical explanation about this change in the commit message (what was the issue with current code, why/how this commit fixes it). This is not exactly trivial, especially since current Curves data layout 'embeds' two other sub-layers of data (so writing the root ID struct also implicitly writes the CurvesGeometry data, which writes the CustomData)...

On a general level, I would also rather have a single CurvesGeometryBlendWriteData blend_write_data variable, generated by a new CurvesGeometry::blend_write_prepare() method, and passed as single third extra argument to CurvesGeometry::blend_write() (would also keep the ID as second argument here btw).

That way, the logic is easier to extend in the future if needs be, and is not spread/duplicated in several parts of the code.

There should be a proper technical explanation about this change in the commit message (what was the issue with current code, why/how this commit fixes it). This is not exactly trivial, especially since current Curves data layout 'embeds' two other sub-layers of data (so writing the root ID struct also implicitly writes the `CurvesGeometry` data, which writes the `CustomData`)... On a general level, I would also rather have a single `CurvesGeometryBlendWriteData blend_write_data` variable, generated by a new `CurvesGeometry::blend_write_prepare()` method, and passed as single third extra argument to `CurvesGeometry::blend_write()` (would also keep the ID as second argument here btw). That way, the logic is easier to extend in the future if needs be, and is not spread/duplicated in several parts of the code.

Falk David added 1 commit 2023-09-12 14:42:46 +02:00

b68195da0c Add `blend_write_prepare` function

Falk David requested review from Bastien Montagne 2023-09-12 14:47:59 +02:00

Bastien Montagne approved these changes 2023-09-12 14:54:10 +02:00

Bastien Montagne left a comment

Generally LGTM now, would rather have actual C++ experts give it another pass though.

Left a couple of comments, but generally the structure of the new code looks good to me, so don't think another review would be necessary from me at this point.

Generally LGTM now, would rather have actual C++ experts give it another pass though. Left a couple of comments, but generally the structure of the new code looks good to me, so don't think another review would be necessary from me at this point.

source/blender/blenkernel/BKE_curves.hh Outdated

						
				@ -399,0 +409,4 @@

				   * This function needs to be called before `blend_write` and before the `CurvesGeometry` struct

				   * is written because it can mutate the `CustomData` struct.

				   */

				  void blend_write_prepare(CurvesGeometryBlendWriteData &write_data);

Bastien Montagne commented

2023-09-12 14:49:53 +02:00

Not a C++ expert, but can't this function simply return a CurvesGeometryBlendWriteData object? I would expect a copy elision to happen here (or at worse, a copy using the 'move' mechanism)?

Not a C++ expert, but can't this function simply return a `CurvesGeometryBlendWriteData` object? I would expect a copy elision to happen here (or at worse, a copy using the 'move' mechanism)?

👍 1

filedescriptor marked this conversation as resolved

source/blender/blenkernel/BKE_curves.hh Outdated

						
				@ -399,0 +410,4 @@

				   * is written because it can mutate the `CustomData` struct.

				   */

				  void blend_write_prepare(CurvesGeometryBlendWriteData &write_data);

				  void blend_write(BlendWriter &writer, const CurvesGeometryBlendWriteData &write_data ID &id);

Bastien Montagne commented

2023-09-12 14:45:44 +02:00

missing coma?...

Also, please pass the ID argument first (just after the writer one), it remains the 'main' data to be written, additional data specific to a data types should be added after it.

missing coma?... Also, please pass the ID argument first (just after the `writer` one), it remains the 'main' data to be written, additional data specific to a data types should be added after it.

filedescriptor marked this conversation as resolved

Bastien Montagne requested review from Hans Goudey 2023-09-12 14:54:22 +02:00

Hans Goudey reviewed 2023-09-12 14:56:38 +02:00

source/blender/blenkernel/BKE_curves.hh Outdated

						
				@ -54,0 +54,4 @@

				/**

				 * Helper struct for `CurvesGeometry::blend_write_*` functions.

				 */

				struct CurvesGeometryBlendWriteData {

Hans Goudey commented

2023-09-12 14:56:36 +02:00

This struct could be defined as BlendWriteData just before the blend_write_prepare function, inside bke::CurvesGeometry

This struct could be defined as `BlendWriteData` just before the `blend_write_prepare` function, inside `bke::CurvesGeometry`

👍 1

filedescriptor marked this conversation as resolved

Falk David added 1 commit 2023-09-12 15:15:05 +02:00

67d94f38b6 Refactor and fixes

Hans Goudey requested changes 2023-09-12 15:24:31 +02:00

source/blender/blenkernel/BKE_curves.hh Outdated

						
				@ -394,3 +394,3 @@

				   * File Read/Write.

				   */

				 public:

Hans Goudey commented

2023-09-12 15:23:35 +02:00

Is public necessary here? Seems the stuff above is already public

filedescriptor marked this conversation as resolved

source/blender/blenkernel/intern/curves.cc

						
				@ -105,12 +105,16 @@ static void curves_blend_write(BlendWriter *writer, ID *id, const void *id_addre

				{

				  Curves *curves = (Curves *)id;

				  /* Make sure to call `CurvesGeometry::blend_write_prepare()` before writing the struct. */

Hans Goudey commented

2023-09-12 15:24:27 +02:00

Think this comment is redundant with documentation in blend_write_prepare and the face that blend_write requires this as an argument. Same in the GP file

Think this comment is redundant with documentation in `blend_write_prepare` and the face that `blend_write` requires this as an argument. Same in the GP file

filedescriptor marked this conversation as resolved

Falk David added 2 commits 2023-09-12 15:38:12 +02:00

cddc90e64a Remove comments

e05597ff92 Remove duplicate `public:`

Hans Goudey approved these changes 2023-09-12 15:46:59 +02:00

source/blender/blenkernel/intern/grease_pencil.cc Outdated

						
				@ -2054,3 +2055,4 @@

				    switch (drawing_base->type) {

				      case GP_DRAWING: {

				        GreasePencilDrawing *drawing = reinterpret_cast<GreasePencilDrawing *>(drawing_base);

				        blender::bke::CurvesGeometry::BlendWriteData write_data =