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.

Sign up for GitHub

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

Jump to bottom

Render context lost handling pt.1 #8900

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft

Render context lost handling pt.1 #8900

wants to merge 3 commits into from

Conversation

ekharkunov
Copy link
Contributor

Technical details

  • Hook gl context events on JS side.
  • Add render.set_listener API.
  • Implement listener notification about gl context events.
  • Add rendering pause if "context_lost" event occurred.

PR checklist

  • Code
    • Add engine and/or editor unit tests.
    • New and changed code follows the overall code style of existing code
    • Add comments where needed
  • Documentation
    • Make sure that API documentation is updated in code comments
    • Make sure that manuals are updated (in github.com/defold/doc)
  • Prepare pull request and affected issue for automatic release notes generator
    • Pull request - Write a message that explains what this pull request does. What was the problem? How was it solved? What are the changes to APIs or the new APIs introduced? This message will be used in the generated release notes. Make sure it is well written and understandable for a user of Defold.
    • Pull request - Write a pull request title that in a sentence summarises what the pull request does. Do not include "Issue-1234 ..." in the title. This text will be used in the generated release notes.
    • Pull request - Link the pull request to the issue(s) it is closing. Use on of the approved closing keywords.
    • Affected issue - Assign the issue to a project. Do not assign the pull request to a project if there is an issue which the pull request closes.
    • Affected issue - Assign the "breaking change" label to the issue if introducing a breaking change.
    • Affected issue - Assign the "skip release notes" is the issue should not be included in the generated release notes.

Example of a well written PR description:

  1. Start with the user facing changes. This will end up in the release notes.
  2. Add one of the GitHub approved closing keywords
  3. Optionally also add the technical changes made. This is information that might help the reviewer. It will not show up in the release notes. Technical changes are identified by a line starting with one of these:
    1. ### Technical changes
    2. Technical changes:
    3. Technical notes:
There was a anomaly in the carbon chroniton propeller, introduced in version 8.10.2. This fix will make sure to reset the phaser collector on application startup.

Fixes #1234

### Technical changes
* Pay special attention to line 23 of phaser_collector.clj as it contains some interesting optimizations
* The propeller code was not taking into account a negative phase.

@ekharkunov ekharkunov force-pushed the issue-8200-render-context-handling-pt1 branch 4 times, most recently from ba9c476 to c54cc25 Compare May 13, 2024 08:43
@ekharkunov ekharkunov force-pushed the issue-8200-render-context-handling-pt1 branch from c54cc25 to 13fd2a7 Compare May 30, 2024 19:40
JCash
JCash reviewed Jun 4, 2024
View reviewed changes
engine/gameobject/proto/gameobject/gameobject_ddf.proto
Comment on lines +288 to +290
message RenderContextLost
{
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this only for game objects?
I kind of would have expected render_ddf.proio to own a "render context lost" message.

engine/resource/src/resource.cpp
Comment on lines +1296 to +1306
bool InvalidateIterator(const IteratorResource& resource, void* user_ctx)
{
HFactory factory = (HFactory)user_ctx;
InvalidateGraphicsHandle(factory, resource.m_Resource);
return true;
}

void InvalidateGraphicsResources(HFactory factory)
{
IterateResources(factory, &InvalidateIterator, factory);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these only used in this file?
Then we prefer to add "static" to the functions, to make them private. (for readability and possible compiler/linker optimizations)

engine/gameobject/src/gameobject/gameobject_props.cpp
Comment on lines +132 to +138
void InvalidatePropertyResources(dmResource::HFactory factory, dmArray<void*>& resources)
{
for (uint32_t i = 0; i < resources.Size(); ++i)
{
dmResource::InvalidateGraphicsHandle(factory, resources[i]);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a little bit too granular.
Wouldn't it work the resource system to just go through all resources in a single for-loop?

engine/gamesys/src/gamesys/components/comp_tilegrid.cpp
TileGridWorld* world = (TileGridWorld*) params.m_World;
if (world->m_VertexDeclaration)
{
// dmGraphics::DeleteVertexDeclaration(world->m_VertexDeclaration);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dead code

engine/gamesys/src/gamesys/components/comp_tilegrid.cpp
if (world->m_VertexDeclaration)
{
// dmGraphics::DeleteVertexDeclaration(world->m_VertexDeclaration);
dmRender::InvalidateBufferedRenderBuffer(world->m_VertexBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The index buffer needs invalidation too?

And, this is my biggest concern with this change, and it's that it requires a lot of detailed fixes.
But, just as dmResource can invalidate all graphics resources, perhaps dmRender could too?
I haven't discussed it with @Jhonnyg but, but my gut feeling is that it's a better place to handle it all.

Copy link
Contributor Author

@ekharkunov ekharkunov Jun 4, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, buffer's handle should be invalidate too. I got errors during testing when first time like 'buffer object relates to another context'. And after a couple of lost/restore cycles it crashed on glDeleteBuffersARB.

But, just as dmResource can invalidate all graphics resources, perhaps dmRender could too?

I'll think about it.

engine/gamesys/src/gamesys/resources/res_fragment_program.cpp
@@ -17,7 +17,7 @@

namespace dmGameSystem
{
static dmResource::Result AcquireResources(dmGraphics::HContext context, dmResource::HFactory factory, dmGraphics::ShaderDesc* ddf, dmGraphics::HVertexProgram* program)
static dmResource::Result AcquireResources(dmGraphics::HContext context, dmResource::HFactory factory, dmGraphics::ShaderDesc* ddf, dmGraphics::HFragmentProgram* program)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe, classic copy paste?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JCash JCash JCash left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ekharkunov @JCash