Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented a RAII wrapper over IUnknown, for Direct3D #2353

Merged

Conversation

ivan-mogilko
Copy link
Contributor

Resolves #2343.

An automatically releasing pointer for IDirect3D objects.

@ivan-mogilko ivan-mogilko added the context: code fixing/improving existing code: refactor, optimise, tidy... label Mar 9, 2024
@ericoporto
Copy link
Member

I think the code for IUnknownPtr needs to be wrapped in a #if AGS_PLATFORM_OS_WINDOWS clause

@ivan-mogilko
Copy link
Contributor Author

I think the code for IUnknownPtr needs to be wrapped in a #if AGS_PLATFORM_OS_WINDOWS clause

It has zero dependency on windows headers, and may work with anything that has AddRef/Release methods.

But I have second thoughts about this approach, will revise this later.

@ivan-mogilko ivan-mogilko reopened this Mar 9, 2024
@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Mar 9, 2024

Hmm, maybe this is still a not so bad approach.
I found that it's not much different to a variant found in this MS dev article, and they even have a similar method of retrieving internal pointer field address for assignment (their function is called "GetAddressOf", while mine is "Acquire"):
https://learn.microsoft.com/en-us/archive/msdn-magazine/2015/february/windows-with-c-com-smart-pointers-revisited

I still need to reorganize this a bit, and I found there's some bug, occuring when resizing a window (but strangely, not when changing from windowed to fullscreen). I will have to carefully recheck all usages of the new ptr type.

EDIT: since I'm at this, i might also check out why there's constantly a warning about 1 remaining reference to IDirect3D on exit.

@ivan-mogilko ivan-mogilko force-pushed the 362--iunknownptr branch 4 times, most recently from ef9b704 to cd5adb8 Compare March 9, 2024 20:56
@ivan-mogilko ivan-mogilko marked this pull request as ready for review March 9, 2024 20:56
@ericoporto
Copy link
Member

ericoporto commented Mar 9, 2024

I threw a bunch of games on this and they all seem to work.

Small things in smart_ptr.h.

  • You can reduce ComPtr<TInterface> to only ComPtr, it should be the same.
  • in uint64_t ref_cnt = _obj->Release(), ref_cnt can be made const

EDIT: since I'm at this, i might also check out why there's constantly a warning about 1 remaining reference to IDirect3D on exit.

Right, I do still get the WARNING: Not all of the Direct3D resources have been disposed; ID3D ref count: 1 at exit too, but this is maybe a different PR.

The CirrusCI failure just looks like a temporary network bug in their end... Rerunning it should work.

@ericoporto
Copy link
Member

ericoporto commented Mar 10, 2024

Oh, I meant just replacing. The <TInterface> next to ComPtr could omitted.

It can be left there too if it makes things clearer.

Edit: actually perhaps it's better to leave it there, not sure how different compilers see this. I only tested with MSVC. :/

@ivan-mogilko ivan-mogilko merged commit 4ad2b57 into adventuregamestudio:master Mar 14, 2024
20 checks passed
@ivan-mogilko ivan-mogilko deleted the 362--iunknownptr branch March 14, 2024 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context: code fixing/improving existing code: refactor, optimise, tidy...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Smart pointer for Direct3D interfaces
2 participants