I've made some bugfixes, want to discuss their implementation before a pull request

Engine version 4.6.1 - Looking at head, I don’t see these bugs as fixed.

First - the bugs. I discovered them when I modeled my placeholder assets at the correct scale in Maya and then had to adjust the scale later. For instance, my player tank is 15 meters long. My scene at that scale doesn’t work in Unreal because of lighting issues, so I decided to scale it down to 1m=10UU.

That’s fine, Maya allows you to do that automatically on export. It doesn’t do it how I would like, instead adding a 0.1 scale to the objects in the FBX. Unreal also doesn’t handle this how I’d like, and simply keeps the scale, and thus internally messes itself up.

The symptoms of the bug are this: Import a scaled down skeletal mesh. Go into Phat and try to give it some collision bodies. The widget location will be at the wrong place but they’ll render correctly. In the level editor, the collisions will be rendered without scale (so, huge.) Convex hulls will render incorrectly in both views.

NB: I went down a lot of rabbit holes looking at possible FBX import bugs, at Maya export bugs and PhysX mesh generation bugs before I realized it was just the editor rendering incorrectly. It was a good full day before I finally realized that it wasn’t some bug deep in the guts of PhysX.

So, the fixes:

This one fixes the widget location. But that RemoveScaling() was there for a reason. What was that reason?


FVector FPhATEdPreviewViewportClient::GetWidgetLocation() const
{
	if (SharedData->EditingMode == FPhATSharedData::PEM_BodyEdit) /// BODY EDITING ///
	{
		// Don't draw widget if nothing selected.
		if (!SharedData->GetSelectedBody())
		{
			return FVector::ZeroVector;
		}

		int32 BoneIndex = SharedData->EditorSkelComp->GetBoneIndex(SharedData->PhysicsAsset->BodySetup[SharedData->GetSelectedBody()->Index]->BoneName);

		FTransform BoneTM = SharedData->EditorSkelComp->GetBoneTransform(BoneIndex);

**                // Commented out this line.
		//BoneTM.RemoveScaling();
**
		return SharedData->EditorSkelComp->GetPrimitiveTransform(BoneTM, SharedData->GetSelectedBody()->Index, SharedData->GetSelectedBody()->PrimitiveType, SharedData->GetSelectedBody()->PrimitiveIndex, 1.f).GetTranslation();
	}
	else  /// CONSTRAINT EDITING ///
	{
		if (!SharedData->GetSelectedConstraint())
		{
			return FVector::ZeroVector;
		}

		return SharedData->GetConstraintMatrix(SharedData->GetSelectedConstraint()->Index, EConstraintFrame::Frame2, 1.f).GetTranslation();
	}
}

The method right below it, GetWidgetCoordSystem() has the same RemoveScaling() call, but doesn’t seem to require removal. I thought it would. Scaling a box element in Phat is pretty slow, you’d think that would be related to GetWidgetCoordSystem() somehow. Not sure what’s happening there. This bug is still outstanding - why is scaling slow?

The other change related to drawing the primitive (non-convex) collision hulls is this:


void UPhysicsAsset::DrawCollision(FPrimitiveDrawInterface* PDI, const USkeletalMesh* SkelMesh, const TArray<FTransform>& SpaceBases, const FTransform& LocalToWorld, float Scale)
{
	for( int32 i=0; i<BodySetup.Num(); i++)
	{
		int32 BoneIndex = SkelMesh->RefSkeleton.FindBoneIndex( BodySetup*->BoneName );
		
		FColor* BoneColor = (FColor*)( &BodySetup* );

		FTransform BoneTransform = GetSkelBoneTransform(BoneIndex, SpaceBases, LocalToWorld);
**// Commented this out.
//		BoneTransform.SetScale3D(FVector(Scale));
**		BodySetup*->CreatePhysicsMeshes();
		BodySetup*->AggGeom.DrawAggGeom( PDI, BoneTransform, *BoneColor, NULL, false, false, false );
	}
}


Scale was coming in as 1.0. The rendering code that does work in PHAT respects that scale. That said, I have no idea why the code is like this, and I hate making changes to code I don’t fully understand. Any input?

The final fix I made was to get the Convex hulls rendering correctly in Phat and in the editor. They were completely ignoring not only scale but whether they were using the Negative-X version of the convex mesh. However, the fix is messy because I shoehorned it in. This one really needs some dev love to make it fit.

In BodySetup, I added ConvexMeshScaled which seems to be the one we should be using instead of PxConvexMesh. This is generated and saved in BodySetup::AddConvexElemsToRigidActor(). Because of that, I had to change it from const as well as do some other internal changes


        

        // In the header
        // Geometry with scale created when physics mesh is generated. Use THIS for rendering, it might actually work.
        TSharedPtr<physx::PxConvexMeshGeometry> ConvexMeshScaled;

void UBodySetup::AddConvexElemsToRigidActor(PxRigidActor* PDestActor, const FTransform& RelativeTM, const FVector& Scale3D, const FVector& Scale3DAbs, TArray<PxShape*>* NewShapes)
{
	float ContactOffsetFactor, MaxContactOffset;
	GetContactOffsetParams(ContactOffsetFactor, MaxContactOffset);
	PxMaterial* PDefaultMat = GetDefaultPhysMaterial();

	for (int32 i = 0; i < AggGeom.ConvexElems.Num(); i++)
	{
                **//Removed const here. :(**
		**FKConvexElem& ConvexElem = AggGeom.ConvexElems*;**

		if (ConvexElem.ConvexMesh)
		{
			PxTransform PLocalPose;
			bool bUseNegX = CalcMeshNegScaleCompensation(Scale3D, PLocalPose);

                        **// Storing in a shared pointer. It was just a stack allocated object before. I don't like using new(), so TSharedPtr it is.**
**			TSharedPtr<PxConvexMeshGeometry> PConvexGeom = MakeShareable<PxConvexMeshGeometry>(new PxConvexMeshGeometry());
**			PConvexGeom->convexMesh = bUseNegX ? ConvexElem.ConvexMeshNegX : ConvexElem.ConvexMesh;
			PConvexGeom->scale.scale = U2PVector(Scale3DAbs * ConvexElem.GetTransform().GetScale3D().GetAbs());

			ConvexElem.ConvexMeshScaled = PConvexGeom;


			FTransform ConvexTransform = ConvexElem.GetTransform();
			if (ConvexTransform.GetScale3D().X < 0 || ConvexTransform.GetScale3D().Y < 0 || ConvexTransform.GetScale3D().Z < 0)
			{
				UE_LOG(LogPhysics, Warning, TEXT("AddConvexElemsToRigidActor: %s] ConvexElem%d] has negative scale. Not currently supported"), *GetPathNameSafe(GetOuter()), i);
			}
			if (ConvexTransform.IsValid())
			{
				PxTransform PElementTransform = U2PTransform(ConvexTransform * RelativeTM);
				PLocalPose.q *= PElementTransform.q;
				PLocalPose.p = PElementTransform.p;
				PLocalPose.p.x *= Scale3D.X;
				PLocalPose.p.y *= Scale3D.Y;
				PLocalPose.p.z *= Scale3D.Z;

				if (PConvexGeom->isValid())
				{
					PxVec3 PBoundsExtents = PConvexGeom->convexMesh->getLocalBounds().getExtents();

					ensure(PLocalPose.isValid());
					PxShape* NewShape = PDestActor->createShape(*PConvexGeom.Get(), *PDefaultMat, PLocalPose);

					if (NewShapes)
					{
						NewShapes->Add(NewShape);
					}

					const float ContactOffset = FMath::Min(MaxContactOffset, ContactOffsetFactor * PBoundsExtents.minElement());
					NewShape->setContactOffset(ContactOffset);
				}
				else
				{
					UE_LOG(LogPhysics, Warning, TEXT("AddConvexElemsToRigidActor: %s] ConvexElem%d] invalid"), *GetPathNameSafe(GetOuter()), i);
				}
			}
			else
			{
				UE_LOG(LogPhysics, Warning, TEXT("AddConvexElemsToRigidActor: %s] ConvexElem%d] has invalid transform"), *GetPathNameSafe(GetOuter()), i);
			}
		}
		else
		{
			UE_LOG(LogPhysics, Warning, TEXT("AddConvexElemsToRigidActor: ConvexElem is missing ConvexMesh (%d: %s)"), i, *GetPathName());
		}
	}
}



Finally, I had to write a new DrawElemWire function using ConvexMeshScaled instead of ConvexMesh. This is basically a copy of the original, but respecting the scale stored in PxConvexMeshGeometry and using the right PxConvexMesh instead of just defaulting to the first one.


void FKConvexElem::DrawElemWire(FPrimitiveDrawInterface* PDI, const FTransform& ElemTM, const FColor Color) const
{
#if WITH_PHYSX
	TSharedPtr<PxConvexMeshGeometry> pxGeom = ConvexMeshScaled;

 	if (pxGeom.IsValid())
	{
		PxConvexMesh* Mesh = pxGeom->convexMesh;

		FVector geomScale = P2UVector(pxGeom->scale.scale);
		FTransform scaleTF = ElemTM;

		scaleTF.SetScale3D(geomScale);

		// Draw each triangle that makes up the convex hull
		PxU32 NbVerts = Mesh->getNbVertices();
		const PxVec3* Vertices = Mesh->getVertices();

		TArray<FVector> TransformedVerts;
		TransformedVerts.AddUninitialized(NbVerts);
		for (PxU32 i = 0; i < NbVerts; i++)
		{
			TransformedVerts* = scaleTF.TransformPosition(P2UVector(Vertices*));
		}

		const PxU8* PIndexBuffer = Mesh->getIndexBuffer();
		PxU32 NbPolygons = Mesh->getNbPolygons();

		for (PxU32 i = 0; i < NbPolygons; i++)
		{
			PxHullPolygon Data;
			bool bStatus = Mesh->getPolygonData(i, Data);
			check(bStatus);

			const PxU8* PIndices = PIndexBuffer + Data.mIndexBase;

			for (PxU16 j = 0; j < Data.mNbVerts; j++)
			{
				// Get the verts that make up this line.
				int32 I0 = PIndices[j];
				int32 I1 = PIndices[j + 1];

				// Loop back last and first vertices
				if (j == Data.mNbVerts - 1)
				{
					I1 = PIndices[0];
				}

				PDI->DrawLine(TransformedVerts[I0], TransformedVerts[I1], Color, SDPG_World);
			}
		}
	} else
	{
		UE_LOG(LogPhysics, Log, TEXT("FKConvexElem::DrawElemWire : No ConvexMesh, so unable to draw."));
	}
#endif
}

UPDATE:

I found another issue with the hit proxies also not respecting scale. Was a minor, obvious fix:


void FKBoxElem::DrawElemSolid(class FPrimitiveDrawInterface* PDI, const FTransform& ElemTM, float Scale, const FMaterialRenderProxy* MaterialRenderProxy) const
{
	DrawBox(PDI, ElemTM.ToMatrixWithScale(),** Scale * **0.5f * FVector(X, Y, Z), MaterialRenderProxy, SDPG_World );
}

void FKBoxElem::GetElemSolid(const FTransform& ElemTM, float Scale, const FMaterialRenderProxy* MaterialRenderProxy, int32 ViewIndex, FMeshElementCollector& Collector) const
{
	GetBoxMesh(ElemTM.ToMatrixWithScale(), **Scale * **0.5f * FVector(X, Y, Z), MaterialRenderProxy, SDPG_World, ViewIndex, Collector);
}


FKBoxElem::DrawElemWire() respects scale in that exact way, these don’t. Seems like a simple oversight, and fixes the problem with hitproxies rendering badly.

So, comments? Questions?

My concerns: I don’t like having to remove const-ness from AddConvexElemsToRigidActor(). It doesn’t fit now with the rest of its sister methods. I don’t like just commenting out things put in explicitly for unknown reasons.

I’d love some input from engine devs on how to make this fix ready for a pull request.

Another scale related fix. I’m a programmer, not an artist so maybe this is bad practice, but what I am currently doing for my placeholders is using joints for attach points. For instance, my tanks turret attaches to the turret01 joint. That joint doesn’t have a skin cluster, and up until this scaling problem there was no issue.

Now, there’s an issue. I’ve seen some workarounds for it in my Googling, but after some serious experimentation with Maya, I can not get it to do anything but what it wants to do, which is scale the bones. That said, you CAN get it to NOT do that to leaf joints, by using “Segment Scale Compensate” - and it duly doesn’t include “Lcl Scale” in that node. But the UE4 code gets it anyway, from the parent.

So, my fix:

In UnFbx::FFbxImporter::ImportBone line 966



if (!GlobalLinkFoundFlag)
{
	FbxAMatrix mat = Link->EvaluateGlobalTransform();

	if (Link->GetChildCount()==0)
	{
		UE_LOG(LogFbx, Log, TEXT("Bone %s has no skin and no children, so setting scale to Identity."), ANSI_TO_TCHAR(Link->GetName()));
		mat.SetS(FbxVector4(1.0,1.0,1.0));
	}

	GlobalsPerLink[LinkIndex] = mat;
}


This code previously just used EvaluateGlobalTransform(). GlobalLinkFoundFlag (weird name) seems to indicate when false that it couldn’t find any weighted vertices. That’s great, that’s exactly what I want to know.

So it seems like this could be an import option. Something like “Set identity scale on unskinned bones” or some such thing.

In any case, it fixes my issue with my attachments shrinking.

Bump.

Is this not the appropriate venue to discuss this, devs?

I think you should actually go ahead and make a pull request - there’s much better chance that somebody from Epic will notice this and answer. :slight_smile:

Ya seems like. I’ll have to update to 4.7 first.

I’ve created a pull request for the rendering fixes against release. I’ve pointed to this thread in the pull request, hopefully there’s some discussion here.

Strangely enough, the two fixes to FKBoxElem were apparently already made on November 20th. Odd that they weren’t in 4.6.1. My git-fu isn’t good enough to tell when that change got integrated into a release.

I’m going to polish up the FBX import change and submit it as well, as I find it quite handy.

Hi Backov,
Just wanted to post on here and thank you for digging into all these.

I’ve got your pull request as one of my upcoming tasks so I’m hoping to get to it soon.