New picking framework by ccpisheden · Pull Request #96 · carbonengine/trinity

ccpisheden · 2026-09-21T15:55:34Z

Summary

Linked issue (optional)

Testing

AI assistance

Checklist

Read CONTRIBUTING.md.
- Commits follow the commit-message style described there.
- Added or updated tests where it made sense.
- Updated docs / inline API comments for any behaviour change.

This reverts commit 8d10608.

This reverts commit a37840a.

This reverts commit 12f72ae.

ccpgames-carbon · 2026-09-21T15:55:41Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

…n interface

Copilot

Copilot review overview

🟡 Changes recommended

Readback lifetime and region handling defects, duplicate instance identifiers, and changed object-resolution behavior can produce invalid or incorrect picking results.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 · 3 · 1

Open (6)

Defer DX12 readback resource release until GPU completion · New
Reject invalid backend readbacks before mapping · New
Handle BeginRenderContext failure before issuing readback commands · New
Advance picking indices across merged mesh additions · New
Initialize stub readbacks from the source texture data · New
Add asynchronous readback coverage for regions and lifetimes · New

What changed in this PR

Introduces asynchronous GPU readbacks and expands picking results to include 64-bit object pointers plus two additional data values.

Changes:

Adds cross-backend Tr2ReadbackAL support.
Reworks scene/debug/instanced-mesh picking and Python APIs.
Propagates 64-bit picking identifiers through render data.

File	Description
`trinityal/stub/Tr2TextureALStub.h`	Declares stub readback support.
`trinityal/stub/Tr2TextureALStub.cpp`	Implements stub readbacks.
`trinityal/src/Tr2TextureAL.cpp`	Adds public readback wrapper.
`trinityal/metal/Tr2TextureALMetal.mm`	Implements Metal readbacks.
`trinityal/metal/Tr2TextureALMetal.h`	Declares Metal readback resources.
`trinityal/include/Tr2TextureAL.h`	Exposes the readback API.
`trinityal/dx12/Tr2TextureALDx12.h`	Declares DX12 readback resources.
`trinityal/dx12/Tr2TextureALDx12.cpp`	Implements DX12 readbacks.
`trinityal/dx11/Tr2TextureALDx11.h`	Declares DX11 readback resources.
`trinityal/dx11/Tr2TextureALDx11.cpp`	Implements DX11 readbacks.
`trinity/Utilities/BVH.cpp`	Adds an explicit index cast.
`trinity/Tr2SkinnedModel.cpp`	Propagates picking pointers.
`trinity/Tr2RenderContext.h`	Stores split picking-pointer variables.
`trinity/Tr2RenderContext.cpp`	Uploads pointer halves and area data.
`trinity/Tr2PickBuffer.h`	Simplifies synchronous mapping API.
`trinity/Tr2PickBuffer.cpp`	Updates pick-buffer mapping.
`trinity/Tr2PerObjectData.h`	Replaces user data with 64-bit picking data.
`trinity/Tr2DebugRenderer.h`	Updates debug picking interface.
`trinity/Tr2DebugRenderer.cpp`	Integrates debug geometry into shared picking.
`trinity/ITr2PickableScene.cpp`	Adopts the updated picking API.
`trinity/Interior/Tr2IntSkinnedObject.cpp`	Propagates picking pointers.
`trinity/Eve/SpaceObject/Children/EveChildMesh.cpp`	Simplifies LOD lookup.
`trinity/Eve/SpaceObject/Children/EveChildInstancedMeshes.cpp`	Adds per-instance picking metadata.
`trinity/Eve/EveSpaceScene.h`	Declares new picking buffers and APIs.
`trinity/Eve/EveSpaceScene.cpp`	Implements the new picking pipeline.
`trinity/Eve/EveSpaceScene_Blue.cpp`	Exposes expanded Python picking results.
`trinity/Eve/EvePicking.h`	Redesigns pending readback state.
`trinity/Eve/EvePicking.cpp`	Stores two extra result values.
`trinity/Eve/EveInstancedMeshManager.h`	Extends instance picking data.
`trinity/Eve/EveInstancedMeshManager.cpp`	Uploads and renders new picking metadata.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

filipppavlov · 2026-09-24T12:52:19Z

+	{
+		m_readScratch->Unmap( 0, nullptr );
+	}
+	m_readScratch = nullptr;


Looks like it is an issue for reals

+	if( !m_readback )
+	{
+		return E_INVALIDCALL;
+	}
+	return m_readback->Map( pointer, rowPitch, renderContext );



-		CR_RETURN( Tr2Renderer::BeginRenderContext() );
-		ON_BLOCK_EXIT( [&] { Tr2Renderer::EndRenderContext(); } );
+			Tr2Renderer::BeginRenderContext();


+			instanceData.pickingMeshIndex = pickingMeshIndex;
+			instanceData.pickingInstanceIndex = static_cast<uint32_t>( i );


+	std::shared_ptr<Tr2ReadbackAL> readback = std::make_shared<Tr2ReadbackAL>();
+	readback->Initialize( size, mipPitch ); //mipPitch here is probably wrong.



 	bool operator==( const Tr2TextureAL& other ) const;

+	Tr2ReadbackAL CreateReadback( const Tr2TextureSubresource& region, Tr2PrimaryRenderContextAL& renderContext );


filipppavlov · 2026-09-24T12:29:09Z

+class Tr2ReadbackAL : public Tr2DeviceResourceAL<Tr2ReadbackAL>
+{
+public:
+	void Initialize( size_t size, uint32_t rowPitch );


Minor: missed this one Initialize->constructor

filipppavlov · 2026-09-24T12:37:15Z

 		if( !objectOffsets.empty() )
 		{
-			objects.reserve( objectOffsets.size() );
+			blueObjects.reserve( objectOffsets.size() );


Minor: the draw is called twice for lines and triangles with the same blueObjects argument, so this reserve seems misplaced. Would it be better to move it to the caller: line 811?

filipppavlov · 2026-09-24T12:39:51Z

 }

 // ------------------------------------------------------------------------------------------------------
 void EvePendingPickingReadback::ReleaseResources( TriStorage s )


Since both ReleaseResources and OnPrepareResources are empty, the class doesn't need to inherit from Tr2DeviceResource

filipppavlov · 2026-09-24T12:41:58Z

 			EveInstancedMeshManager::StaticPerInstanceData instanceData;
 			instanceData.worldTransform = Float4x3( instanceTransforms[i] );
 			instanceData.sphereIndex = static_cast<uint32_t>( existingCount + i );
+			instanceData.pickingMeshIndex = pickingMeshIndex;


This line, and the next one, look incorrect: we are adding instances to an already existing mesh

ccpisheden added 11 commits September 8, 2026 16:53

Initial picking work

bab8eb2

Merge branch 'main' into picking

76bca31

Fixed picking with the instancing and debug renderer changes.

8d10608

Mac async picking stuff

1e9deb3

Mac compilation warning fixes

010537d

Metal compilation error fixes

94f9000

Revert "Fixed picking with the instancing and debug renderer changes."

a37840a

This reverts commit 8d10608.

Revert "Fixed picking with the instancing and debug renderer changes."

12f72ae

This reverts commit 8d10608.

Reapply "Fixed picking with the instancing and debug renderer changes."

1d78385

This reverts commit a37840a.

Reapply "Fixed picking with the instancing and debug renderer changes."

56277d0

This reverts commit 12f72ae.

Commented out debug code.

48f8409

ccpisheden added 5 commits September 21, 2026 18:05

Cleanup.

b404bc5

Fixed picking unmapping not being correct.

c342629

Removed Readback initialize functions, added null checks, fixed Pytho…

8cf68be

…n interface

Fixed Mac picking.

b3d1143

Clang formatting fixes for picking.

792366f

ccpisheden changed the title ~~Picking~~ New picking framework Sep 23, 2026

ccpisheden added 2 commits September 23, 2026 18:24

Merge branch 'main' into picking

c92c202

Removed picking log spam.

1b95ad2

ccpisheden marked this pull request as ready for review September 23, 2026 16:40

filipppavlov requested a balanced review from Copilot September 24, 2026 11:53

Copilot started reviewing on behalf of filipppavlov September 24, 2026 11:54 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

filipppavlov reviewed Sep 24, 2026

View reviewed changes

Navigation Menu

New picking framework - #96

New picking framework#96
ccpisheden wants to merge 18 commits into
mainfrom
picking

ccpisheden commented Sep 21, 2026

Uh oh!

ccpgames-carbon commented Sep 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

filipppavlov Sep 24, 2026

Uh oh!

filipppavlov Sep 24, 2026

Uh oh!

filipppavlov Sep 24, 2026

Uh oh!

filipppavlov Sep 24, 2026

Uh oh!

filipppavlov Sep 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		instanceData.pickingMeshIndex = pickingMeshIndex;
		instanceData.pickingInstanceIndex = static_cast<uint32_t>( i );

		std::shared_ptr<Tr2ReadbackAL> readback = std::make_shared<Tr2ReadbackAL>();
		readback->Initialize( size, mipPitch ); //mipPitch here is probably wrong.


		bool operator==( const Tr2TextureAL& other ) const;

		Tr2ReadbackAL CreateReadback( const Tr2TextureSubresource& region, Tr2PrimaryRenderContextAL& renderContext );

Conversation

ccpisheden commented Sep 21, 2026

Summary

Linked issue (optional)

Testing

AI assistance

Checklist

Uh oh!

ccpgames-carbon commented Sep 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

filipppavlov Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

filipppavlov Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

filipppavlov Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

filipppavlov Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

filipppavlov Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants