Page MenuHome

Fix T54270: Reset last_hit and last_location when reading the file
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Wed, Feb 5, 12:41 AM.

Details

Summary

It does not make sense to read those values when loading a file and they can crash the cursor if they contain invalid coordinates.

Diff Detail

Repository
rB Blender

Event Timeline

If the problem is with ups->last_location then I believe it is the one that should be corrected and not location. No?
If it is written when reading the file, then in my opinion, the readfile that should check this.
But I don't know the code much, (I don't know what's involved).

Think about doing this in the readfile:

diff --git a/source/blender/blenloader/intern/readfile.c b/source/blender/blenloader/intern/readfile.c
index 2ad79b4e252..ab4103e731c 100644
--- a/source/blender/blenloader/intern/readfile.c
+++ b/source/blender/blenloader/intern/readfile.c
@@ -6627,6 +6627,15 @@ static void direct_link_scene(FileData *fd, Scene *sce)
 
   sce->toolsettings = newdataadr(fd, sce->toolsettings);
   if (sce->toolsettings) {
+    {
+      /* Sanity check. */
+      struct UnifiedPaintSettings *ups = &sce->toolsettings->unified_paint_settings;
+      if (!is_finite_v3(ups->last_location)) {
+        /* Can crash the paint cursor. */
+        zero_v3(ups->last_location);
+      }
+    }
+
     direct_link_paint_helper(fd, sce, (Paint **)&sce->toolsettings->sculpt);
     direct_link_paint_helper(fd, sce, (Paint **)&sce->toolsettings->vpaint);
     direct_link_paint_helper(fd, sce, (Paint **)&sce->toolsettings->wpaint);

But it doesn't seem safe to trust a value that can be changed externally.
So the patch solution seems to be better.

We could always set both last_hit and last_location to zero on file load. It doesn't make a lot of sense to me for that to be preserved?

Jeroen Bakker (jbakker) requested changes to this revision.Mon, Feb 17, 3:25 PM

I agree with brecht. Initializing during load is the best option as it we need to address fewer exceptions during painting

This revision now requires changes to proceed.Mon, Feb 17, 3:25 PM
  • Revert "Fix T54270: Ensure that the vector read from ups->last_location is valid"
  • Reset last_hit and last_location when reading the file
Pablo Dobarro (pablodp606) retitled this revision from Fix T54270: Ensure that the vector read from ups->last_location is valid to Fix T54270: Reset last_hit and last_location when reading the file.Wed, Feb 19, 4:51 PM
Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenloader/intern/readfile.c
6607

Improve the description:

/* Reset last_location and last_hit, so they are not remembered across sessions.
  * In some files these are also NaN, which could lead to crashes in painting. */
This revision was not accepted when it landed; it landed in state Needs Review.Wed, Feb 19, 7:03 PM
This revision was automatically updated to reflect the committed changes.