aws / lumberyard

Amazon Lumberyard is a free AAA game engine deeply integrated with AWS and Twitch – with full source.
Other
2.02k stars 539 forks source link

Bug fix for extra movement of vegetation instances #521

Open yuriy0 opened 3 years ago

yuriy0 commented 3 years ago

Issue #, if available:

Description of changes:

This fixes a bug where moving a vegetation instance with the movement gizmo while in Vegetation Tool moves the instance much more than the gizmo moves, i.e. the gizmo position and the instance position do not correspond.

The cause is as follows: AxisGizmo is calling CEditTool::OnManipulatorDrag with a change in position relative to the manipulator position when the manipulator was first started dragging, e.g.:

Vec3 p1 = view->SnapToGrid(view->ViewToWorld(m_cMouseDownPos));
Vec3 p2 = view->SnapToGrid(view->ViewToWorld(point));
vDragValue = p2 - p1;

(AxisGizmo.cpp:417) The actual method of computing this drag value differs based on the edit mode settings, but vDragValue is always relative to the position of the manipulator at drag start.

Then CVegetationTool::MoveSelected takes that offset exactly as is and applies it to the current position of the vegetation instance:

void CVegetationTool::MoveSelected(CViewport* view, const Vec3& offset, bool bFollowTerrain)
{
    ....
    for (int i = 0; i < m_selectedThings.size(); i++)
    {
        oldPos = m_selectedThings[i].pInstance->pos;

        ...

        newPos = oldPos + offset;
        ...

The result of this logic is, for example:

  1. Start dragging the translation gizmo at (500,500,32), which is the position of the vegetation instance.
  2. On the 1st mouse move callback you drag it to (500, 501, 32). The offset from start position is (0,1,0).
  3. CVegetationTool::MoveSelected moves the instance to (500, 500, 32) + (0, 1, 0) = (500, 501, 32) (correct so far)
  4. On the 2nd mouse move callback you drag the gizmo to (500, 502, 32). The offset from the start position is (0, 2, 0). CVegetationTool::MoveSelected moves the instance to (500, 501, 32) (it’s current position) + (0, 2, 0) = (500, 503, 32) (NOT the position of the gizmo!)

Generally this results in moving the instance way too far compared to the gizmo position (indeed, the difference between actual and intended end positions is exponential).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

AMZN-alexpete commented 3 years ago

Thanks for submitting this fix @yuriy0, we're working on getting it merged for an upcoming release.