Skip to content

2.6 debug #1040

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

Open
cedricp wants to merge 3 commits into
base: RB-2.6
Choose a base branch
from
Open

2.6 debug #1040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28f1264
Build libs install
cedricp Apr 25, 2025
ea2e028
Attempt to fix undocking issue
cedricp Apr 25, 2025
0615c75
Add viewer dithering
cedricp May 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions App/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@
# ***** END LICENSE BLOCK *****

set(Natron_SOURCES NatronApp_main.cpp)
if(WINDOWS)
list(APPEND Natron_SOURCES ../Natron.rc)
endif()
add_executable(Natron ${Natron_SOURCES})
if(WIN32)
Copy link
Copy Markdown
Member

@devernay devernay May 20, 2025

Choose a reason for hiding this comment

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

I can't review the WIN32 stuff, @acolwell can you give it a look?

Copy link
Copy Markdown
Member

@devernay devernay May 20, 2025

Choose a reason for hiding this comment

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

Also it would be nice to have two separate PRs (one for the CMake stuff, one for the OpenGL stuff)

Copy link
Copy Markdown
Collaborator

@acolwell acolwell May 20, 2025

Choose a reason for hiding this comment

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

I agree. The CMake stuff should be in its own PR so it can be landed quickly. I mentioned this in the previous incarnation of these changes.

target_sources(Natron PRIVATE ../Natron.rc)
endif()
if(APPLE)
set_target_properties(Natron PROPERTIES
MACOSX_BUNDLE TRUE
Expand Down Expand Up @@ -59,3 +59,40 @@ install(FILES ../Gui/Resources/Images/natronIcon256_linux.png
DESTINATION "${CMAKE_INSTALL_DATADIR}/pixmaps")
install(FILES ../Gui/Resources/Images/natronProjectIcon_linux.png
DESTINATION "${CMAKE_INSTALL_DATADIR}/pixmaps")

IF(WIN32 AND DEPLOYQT_FOUND)
Copy link
Copy Markdown
Contributor

@rodlie rodlie Jun 3, 2025

Choose a reason for hiding this comment

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

Don't force this, make it optional.

SET(CONFIG_FILE ${CMAKE_CURRENT_BINARY_DIR}/install_deps_${CMAKE_BUILD_TYPE}.cmake)

# Find and install needed DLLs
file(GENERATE OUTPUT
"${CONFIG_FILE}" CONTENT
[[
EXECUTE_PROCESS(COMMAND del ${INSTALL_BINPATH}/*.dll)
SET(TARGET_APP $<TARGET_FILE:Natron>)
FILE(GET_RUNTIME_DEPENDENCIES
RESOLVED_DEPENDENCIES_VAR deps_resolved
UNRESOLVED_DEPENDENCIES_VAR deps_unresolved
EXECUTABLES ${TARGET_APP}
DIRECTORIES ${CMAKELIBPATH}
PRE_EXCLUDE_REGEXES "api-ms-*" "ext-ms-*"
POST_EXCLUDE_REGEXES ".*system32/.*\\.dll"
)
MESSAGE(STATUS "Resolving runtime dependencies for ${TARGET_APP}")
FOREACH(dep ${deps_resolved})
FILE(INSTALL ${dep} DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)
MESSAGE(STATUS "Installing ${dep}")
ENDFOREACH()
FOREACH(dep ${deps_unresolved})
MESSAGE(WARNING "Runtime dependency ${dep} could not be resolved.")
ENDFOREACH()
EXECUTE_PROCESS(COMMAND ${DEPLOYQT_EXE} --no-angle --compiler-runtime ${INSTALL_BINPATH}/Natron.exe)
]])

INSTALL(CODE "SET(INSTALL_BINPATH \"${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_BINDIR}\")")
INSTALL(CODE "SET(DEPLOYQT_EXE \"${DEPLOYQT_EXE}\")")
INSTALL(CODE "SET(CMAKELIBPATH \"${CMAKE_SYSTEM_LIBRARY_PATH};${CMAKE_MINGW_SYSTEM_LIBRARY_PATH};${MINGWPATH}\")")
INSTALL(SCRIPT ${CONFIG_FILE})
INSTALL(DIRECTORY ${Python3_LIBRARY_DIRS}/python${Python3_VERSION_MAJOR}.${Python3_VERSION_MINOR}
DESTINATION lib
PATTERN "*.pyc" EXCLUDE)
ENDIF()
11 changes: 11 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ if(WIN32)
endif()
find_package(Python3 COMPONENTS Interpreter Development)

if(WIN32)
# Search qtdeploy executable
find_program(DEPLOYQT_EXE windeployqt.exe)
if(NOT ${DEPLOYQT_EXE} STREQUAL "")
set(DEPLOYQT_FOUND 1)
else()
set(DEPLOYQT_FOUND 0)
message(STATUS "windeployqt.exe not found, will not deploy Qt dependencies")
endif()
endif()

if(IS_DEBUG_BUILD AND WIN32)
# Explicitly setting SHIBOKEN_PYTHON_LIBRARIES variable to avoid PYTHON_DEBUG_LIBRARY-NOTFOUND
# link errors on Windows debug builds.
Expand Down
3 changes: 3 additions & 0 deletions Engine/AppManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,8 @@ AppManager::loadFromArgs(const CLArgs& cl)
std::cout << "argv[" << i << "] = " << StrUtils::utf16_to_utf8( std::wstring(_imp->commandLineArgsWide[i]) ) << std::endl;
}
#endif
// This should fix GL widgets when undocked
Copy link
Copy Markdown
Member

@devernay devernay May 20, 2025

Choose a reason for hiding this comment

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

Nice to add a comment, but it would be nice to have a link to the "why". I found this one, maybe you have a better one?

githubuser0xFFFF/Qt-Advanced-Docking-System#397 (comment)

QCoreApplication::setAttribute(Qt::AA_ShareOpenGLContexts);

// This needs to be done BEFORE creating qApp because
// on Linux, X11 will create a context that would corrupt
Expand All @@ -323,6 +325,7 @@ AppManager::loadFromArgs(const CLArgs& cl)
_imp->renderingContextPool.reset( new GPUContextPool() );
initializeOpenGLFunctionsOnce(true);


Copy link
Copy Markdown
Member

@devernay devernay May 20, 2025

Choose a reason for hiding this comment

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

Please avoid formatting changes in PRs (or make a formatting-only PR)

// QCoreApplication will hold a reference to that appManagerArgc integer until it dies.
// Thus ensure that the QCoreApplication is destroyed when returning this function.
initializeQApp(_imp->nArgs, &_imp->commandLineArgsUtf8.front()); // calls QCoreApplication::QCoreApplication(), which calls setlocale()
Expand Down
2 changes: 2 additions & 0 deletions Global/Enums.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,8 @@ enum PixmapEnum
NATRON_PIXMAP_VIEWER_GAMMA_DISABLED,
NATRON_PIXMAP_VIEWER_GAIN_ENABLED,
NATRON_PIXMAP_VIEWER_GAIN_DISABLED,
NATRON_PIXMAP_VIEWER_DITHER_ENABLED,
NATRON_PIXMAP_VIEWER_DITHER_DISABLED,

NATRON_PIXMAP_SCRIPT_CLEAR_OUTPUT,
NATRON_PIXMAP_SCRIPT_EXEC_SCRIPT,
Expand Down
1 change: 1 addition & 0 deletions Gui/CurveWidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ CurveWidget::initializeGL()
// always running in the main thread
assert( qApp && qApp->thread() == QThread::currentThread() );
appPTR->initializeOpenGLFunctionsOnce();
makeCurrent();
Copy link
Copy Markdown
Member

@devernay devernay May 20, 2025

Choose a reason for hiding this comment

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

I don't understand why you call makeCurrent here.

From the doc: There is no need to call makeCurrent() because this has already been done when this function is called.

Copy link
Copy Markdown
Collaborator

@acolwell acolwell May 20, 2025

Choose a reason for hiding this comment

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

So these calls are needed because initializeOpenGLFunctionsOnce() can end up creating a context, making it current, and then does not restore the old context. I believe we should fix that behavior instead of papering over it here and below. I started looking into that but haven't got a full solution yet. I also am a little skeptical about the need to init all the GL functions in these various places. ISTM that this should happen very early in startup instead of here, but I'm not as familiar with this code yet.

I suspect making something like my ScopedGLContext on Windows cross-platform would make resolving this and any other similar situation we encounter simpler.

Copy link
Copy Markdown
Contributor Author

@cedricp cedricp May 21, 2025

Choose a reason for hiding this comment

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

I agree, I thought it was a good idea to push what I found to help maintainers debug the code.

}

void
Expand Down
2 changes: 2 additions & 0 deletions Gui/DopeSheetView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3171,6 +3171,8 @@ DopeSheetView::initializeGL()
return;
}

makeCurrent();
Copy link
Copy Markdown
Member

@devernay devernay May 20, 2025

Choose a reason for hiding this comment

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

same as above, no need to call makeCurrent, according to the doc

Copy link
Copy Markdown
Contributor Author

@cedricp cedricp May 30, 2025

Choose a reason for hiding this comment

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

You're right, I think it's something I left while debugging


_imp->generateKeyframeTextures();
}

Expand Down
6 changes: 6 additions & 0 deletions Gui/GuiApplicationManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,12 @@ GuiApplicationManager::getIcon(PixmapEnum e,
path = NATRON_IMAGES_PATH "interp_curve_z.png";
break;
// DON'T add a default: case here
case NATRON_PIXMAP_VIEWER_DITHER_ENABLED:
path = NATRON_IMAGES_PATH "dither_enabled.png";
break;
case NATRON_PIXMAP_VIEWER_DITHER_DISABLED:
path = NATRON_IMAGES_PATH "dither_disabled.png";
break;
} // switch
if ( path.empty() ) {
assert(!"Missing image.");
Expand Down
2 changes: 2 additions & 0 deletions Gui/GuiResources.qrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@
<file>Resources/Images/curve.png</file>
<file>Resources/Images/cuspPoints.png</file>
<file>Resources/Images/diskcache_icon.png</file>
<file>Resources/Images/dither_enabled.png</file>
<file>Resources/Images/dither_disabled.png</file>
<file>Resources/Images/dot_icon.png</file>
<file>Resources/Images/ellipse.png</file>
<file>Resources/Images/enter_group.png</file>
Expand Down
Binary file added Gui/Resources/Images/dither_disabled.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added Gui/Resources/Images/dither_enabled.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Loading