@michael knubben (michaelknubben) @Gottfried Hofmann (gottfried) Thanks. @Jonathan Williamson (carter2422) / @venomgfx / @Julian Eisel (Severin) , any opinions?

@Paweł Łyczkowski (plyczkowski) I also like the clear difference between increment and grid in v2. Nice work! :)
Two things which bother me with the old grid icon are that the blue circle on the sphere has 2x pixels which is kind of visible on high dpi displays and that the center point is slightly off the grid. The latter might be so that the grid is more visible which is understandable.

@Sergey Sharybin (sergey) Regarding line 2476 in transform_snap.c: If I remember correctly the function only got called when the snap mode was SCE_SNAP_MODE_INCREMENT. I changed this so it also gets called when the mode is SCE_SNAP_MODE_GRID. That's why I added the print statement in the else part (it should never get executed). The only thing that's new is the behavior in case of SCE_SNAP_MODE_GRID. So old functionality shouldn't have changed.

I hope I can make another patch for testing once the icon is finalized. :)

@Fabio Arnold (donfabio)

blue circle on the sphere has 2x pixels which is kind of visible on high dpi displays

Please elaborate on that? What do you mean by 2x pixels?

once the icon is finalized

Keep in mind that I will require green light from someone higher up before doing the final vector icon.

@Paweł Łyczkowski (plyczkowski)

@Fabio Arnold (donfabio) Ah, yeah, I see what you mean.

Is this finished? If it is, and is going into master, then I can provide the final icon.

Based on @Sergey Sharybin (sergey)'s comments above I believe it needs another code review. I'd very much like to get this polished up for acceptance, though. It's really a crucial feature for architectural and game level development.

Just tested: the patch still works with current master. :)

As I explained 2.5 months ago, this doesn't break old snapping behavior, because the function which handles grid snapping only gets called when t->tsnap.mode is either SCE_SNAP_MODE_INCREMENT or SCE_SNAP_MODE_GRID. Before the patch the function was only called when t->tsnap.mode was set to SCE_SNAP_MODE_INCREMENT.

Although I didn't really change anything after the first review it would be great, if one of the devs could have a look at this again with regards to my explanation.

Now the only thing I see missing is an icon

Coming soon.

@Fabio Arnold (donfabio), I think what Sergey meant was the if (!(t->con.mode & CON_APPLY) || t->con.mode & (CON_AXIS0 << i)) that seems like it was put there by accident (your last patch version, line 2476).

Edit: Noticed the failed patching mentioned by Jonathan was around that line, so guess it was a merge conflict.

It's quite some time ago but I think I put that line there intentionally. Probably because axis constraints weren't working otherwise.. I'm not sure.

Anyways, thanks for picking up this diff! :) I just tested it and everything works as expected.

Edit: Actually there is one issue: If you go to top view for example and move an object at some random location, then try to move it using absolute grid snapping, the object actually moves in increments...

Edit2: OK, now I know why I put that if-condition there: Absolute grid snapping should snap the object positions of even integers (or decimals when holding shift). If you're using an axis constraint it isn't desired that all the other axis get snapped to even integer, too. The check prevents that from happening. Note: this is not an issue when using incremental snapping, because it doesn't alter the origin of the translation.

Can't recreate the top-view issue here. Are you sure you're using the right mode (they are still using the same icon which is kinda confusing)?

See what you mean re. constraints, however, holding down shift I'd expect it to use the next smaller grid size just like it's already happening without the if (...). This is more a design question I think, but IMHO it's how it should work.

Well, the issue wasn't related specifically to the top view. But when moving from top view in orthographic projection iter_fac will be 0.1 instead of 1.0. And absolute snapping wasn't working with iter_fac < 1.0.

I changed
val[i] = iter_fac * (floorf(t->center[i] + val[i] / (fac[action] * asp[i]) + 0.5f) - t->center[i]);
to
val[i] = iter_fac * floorf((t->center[i] + val[i]) / iter_fac + 0.5f) - t->center[i];
which does the trick.

The reason for the axis constraint thing is basically that if I want to move an object on the x-axis using grid snapping while constraining to the x-axis I don't want it to jump to 4.0 on the y-axis or whatever while it was on 4.4 or something. I think this makes sense.

Here is the Increment icon (for absolute snap use the existing Grid snap icon): https://drive.google.com/file/d/0B8V1o1Fl0ub1SWx6TVQ1TGZPZjQ/view?usp=sharing

Edit: I just checked: The SVG in master also produces these blank icons. I will generate a patch now.

patch is now 4.7 MB... (I simply imported Pawel's icon).

Don't know if that's relevant, but keep in mind that there are hidden layers in my svg - I guess to properly import into blender you have to copy just the icon from my file and paste it properly (do mind how it is placed on the pixels) in the blender's svg.

I noticed that the blank icons came from the macros in UI_icons.h...

In D910#27825, @Paweł Łyczkowski (plyczkowski) wrote:

patch is now 4.7 MB... (I simply imported Pawel's icon).

Don't know if that's relevant, but keep in mind that there are hidden layers in my svg - I guess to properly import into blender you have to copy just the icon from my file and paste it properly (do mind how it is placed on the pixels) in the blender's svg.

I saw that. I deleted all other layers and cleaned up the file using Inkscape's File > Clean up document. Otherwise the import would have taken forever. I also paid attention to line up the lines to the pixel grid. If you want to you can check the SVG. Could be that I missed something. I never did this before..

Did some testing. There's a new problem: scale and rotate are affected by t->center_global and are having offsets based on the position. I think the desired behaviour would be that scale and rotate behave like snap_increment when snap_grid is active. What do you think?

I don't know what to do about other transform orientations than the global one when using the 3D manipulator or any axis constraints. Should the translation be snapped to the grid regardless of any axis constraints?

Re.: Function naming:
These functions execute for grid and grid-increment snapping. Our API-convention is to use the shared term, afaik (so would be snapGrid here). However, these are picky details we can check them later.

Re.: Other transformation orientations:
Yes, it should always snap to grid. If it didn't, it would conflict with what the user set. This might make things a bit more complicated, but IMHO that's a necessary change

And while you're already on it, just another super-minor thingy ;)

Committed rB48ef0501b7cbbd10d5462c68a2559b2b64848b97

Icon still needs to be added.

Regarding the icon order: I had already uploaded an updated blender_icons.svg in the summary, where I grouped the snap icons (and moved older ones to fit) and added Pawel's final icon. Didn't want to add the SVG file to the patch because it was 4MB.

Severin: I agree snap_grid_increment and snap_grid_absolute would be a better naming. Then the function names would also make sense. Thanks for pointing out code style issues. I was a bit sloppy.

Campbell: Thanks for commandeering and commiting but is the behaviour for custom transform orientations resolved? For example: move an object to some uneven location. Switch transform orientation to view and drag an axis of the manipulator. The translation starts off at a weird offset because of t->center_global and ultimately the object doesn't snap to an even location. I think we shouldn't even snap to an even location then because that would void the axis constraint. So my suggestion would be to behave like snap_grid_increment in that case.

As for rotations I see two options:

1. Start at 0° and snap to 5° increments (this is actually relative to the object's current rotation)
2. Get the angle of the current rotation axis of the object and snap it 5° to increments

(1.) is how it currently behaves. (2.) would better fit to the notion of an absolute transform. At least when the rotation is around one of the global axis of the grid.

For scaling I see similiar options and another one would be snapping to the object's dimensions...

But actually I prefer the current behaviour of behaving just like snap_grid_increments for rotation and scaling. What do you guys think?

In D910#28054, @Fabio Arnold (donfabio) wrote:

snap_grid_increment and snap_grid_absolute would be a better naming.

Why snap_grid_increment? The increment option adds increment snapping to many things, like rotation, and has not much to do with the grid. It's incidental when an object is placed at the grid, and moved in increments, that it looks as though it's snapping to the grid.

I would go with snap_increment, "Snap to increments during transform."/"Transform in increments."
and snap_grid "Snap to grid."

Eventually I'd like to see the snapping option as non-exclusive toggles, placed in a quick-settings popup, so you could have both snap to increments and snap to grid, or any other combination.

OK, based on Pawel's comment I have a new proposal which would be a bit more work: first get rid of the new snap_grid_absolute. Only keep snap_increment. Then add a toggle for absolute/relative snapping which is only shown when snap_increment is active. This toggle is only shown for transform orientation/spaces where it makes sense and an origin exists. So only show it in global/local orientation. Normal and view don't have an origin where absolute snapping would make sense AFAIK. When absolute is checked and incremental snapping is active make translation/rotation/scaling work as the second option I mentioned in my previous post. Also respect the orientation/space and don't override it as with snap_grid_absolute right now.

This would add a key thing to blender which it is currently lacking IMO: absolute transforms.

To go even further to be more consistent and complete when absolute transform is active the absolute location/rotation/scale should be displayed during translation/rotation/scaling operations. Right now only the delta of the operation is displayed and you have to look in the properties panel for transform info.

While I looked to get this feature finished up, it seems committing to master opened up new questions on how it might work.
Seems this feature is still too much in discussion, and an open-topic. See T45212.

@Paweł Łyczkowski (plyczkowski), if you think behavior should eventually be changed. It would be good to say so early on.

Disabled this patch in master until theres some more final design rB3d616ccba888ed678642356317745e8b638a908b

In D910#28096, @Campbell Barton (campbellbarton) wrote:

@Paweł Łyczkowski (plyczkowski), if you think behavior should eventually be changed. It would be good to say so early on.

Yup, sorry about that. I may have spoken too hastily, since it's not that obvious how all these modes could work as non-exclusive toggles. I'll add some thoughts in https://developer.blender.org/rB3d616ccba888ed678642356317745e8b638a908b

Note for future reference: This patch is now in master, but accessed as a toggle instead of a new snapping mode.

Added icon for incremental grid snapping rB14dd88e817432b