Various updates for OGRE

#20 Declined
Repository
Deleted repository
Branch
v0-8 (b9713dfdc12c)
Repository
CEGUI
Branch
v0-8
Author
  1. OgreT
Reviewers
Description
  • Remove version number from path names, because of errors in sampleframework
  • Use Ogre-CMake files (SDK) to detect Ogre
  • Remove unused Ogre overlay parts from BaseApplication
  • Update boost detection in main CMakeLists.txt
  • Bugfix (typo) for XMLParserModule.cpp

Comments (8)

  1. Paul Turner

    Will reject for multiple reasons: Don't remove versions from output binaries, they are there for a reason. Don't prefer static libs for windows / apple. Static linking sucks and we will not encourage its use. Most of the rest of the boost/ogre specific stuff disgusts me just to look at it.

    The only things here we would take is the typo fix and the removal of the unused d_statsOverlay.

  2. OgreT author

    I don't use static linking, but default compiling cegui is not working under win7 with VS2010 and VS2012.

    C:\dev\cegui\samples_framework\include\CEGuiOgreBaseApplication.h(35): fatal error C1083: Cannot open include file: 'Ogre.h': No such file or directory
    ..\..\..\..\..\cegui\src\RendererModules\Ogre\ResourceProvider.cpp(33): fatal error C1083: Cannot open include file: 'OgreArchiveManager.h': No such file or directory
    C:\dev\cegui\include\CEGUI/RendererModules/Ogre/GeometryBuffer.h(36): fatal error C1083: Cannot open include file: 'OgreMatrix4.h': No such file or directory
    C:\dev\cegui\include\CEGUI/RendererModules/Ogre/RenderTarget.h(34): fatal error C1083: Cannot open include file: 'OgreMatrix4.h': No such file or directory
    C:\dev\cegui\include\CEGUI/RendererModules/Ogre/Texture.h(33): fatal error C1083: Cannot open include file: 'OgreTexture.h': No such file or directory
    ..\..\samples_framework\src\CEGuiOgreBaseApplication.cpp(40): fatal error C1083: Cannot open include file: 'OgreWindowEventUtilities.h': No such file or directory
    ..\..\..\..\..\cegui\src\XMLParserModules\RapidXML\XMLParserModule.cpp(40): error C2065: 'CEGUI_DETELE_AO' : undeclared identifier
    ..\..\..\..\..\cegui\src\XMLParserModules\RapidXML\XMLParserModule.cpp(40): error C2146: syntax error : missing ';' before identifier 'parser'
    

    After fixing the FindOgre stuff (with boost) and the RapidXML typo:

    C:\dev\cegui\samples_framework\include\CEGuiOgreBaseApplication.h(126): error C2039: 'Overlay' : is not a member of 'Ogre'
    C:\dev\cegui\samples_framework\include\CEGuiOgreBaseApplication.h(126): error C2143: syntax error : missing ';' before '*'
    C:\dev\cegui\samples_framework\include\CEGuiOgreBaseApplication.h(126): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
    C:\dev\cegui\samples_framework\include\CEGuiOgreBaseApplication.h(126): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
    

    After removing OgreOverlay:

    CEGUISampleFramework-0.8: LINK : fatal error LNK1104: cannot open file 'CEGUIOgreRenderer_d.lib'
    

    There is a error in CMake with that stupid numbering system, that's the reason why I removed it. Why cegui_set_library_name macro is using only ${CEGUI_VERSION_MAJOR}? The other macros are using ${CEGUI_VERSION_MAJOR}.${CEGUI_VERSION_MINOR}.

    Ogre: Try to avoid using a selfmade FindOgre! Use the CMake module from the SDK to get all changes. The current ogre release requires a few boost libraries, if they are compiled with boost-threading support. Maybe it looks awful, but it's working.

  3. Paul Turner

    We acknowledge the build errors as far as the CEGUI_DELETE_AO typo and the Overlay issue.

    It's not a "stupid numbering system" by saying things like that you lose credibility. The -0 is added to API stable elements. -0.8 is added to ABI stable elements. This is important for systems where you may have multiple versions of things installed side by side. If there is a build issue, such as you've reported, then the correct thing is to investigate it - not just hack out things that you clearly do not understand. Look at it from our perspective - things are done for a reason. Then someone just "removes" it, without any kind of explanation beyond what is basically "it works for me like this"? What kind of reaction were you expecting?

    Also, you hack the cegui/cmake/FindOgre.cmake file - which is fair enough to change, btw - but then just leave it there hacked. It no longer does anything, it's no longer used in the modifications, so why is it still there? Why not either leave it alone or remove it entirely. The change looks like you attacked things with an axe with little care or thought at all.

    Additionally, there are things like our own check for other boost libraries that we would use later on in the cmake. Why wasn't it all rolled into one neat thing rather than having it spread all over the place? Later on in the cmake file there is also another check for which boost version was found, and that adds system lib for boost >=1.5, part of your change effectively duplicates this. Why did you not refactor this into a single check? See to me it just looks like careless hacking.

    If Ogre requires boost libraries to be linked in then the Ogre CMake module should provide the names of those libs. It should not be up to client code to "guess" or otherwise discover the names of those libs. And if we were to eventually go with something like that (we won't, btw), then it should be separated into a macro or something to make things easier to fix up if Ogre ever gets its act together and stops forcing it's dependencies onto other people.

    Please don't misunderstand me here. I'm well aware that the existing FindOgre that is in CEGUI is nothing more than a temporary measure. I've said this on multiple occasions. But regardless of that, the changes made here are not acceptable to us in their current form.

    Finally, all the changes are in one commit, which is horrible. There are four separate things here: The typo fix, The removal of the unneeded Overlay variable, The removal of the version suffixes, The CMake changes.

    Each of these things should be a single commit. They should not all be in one commit. Again, it just comes over as being highly careless.

    You say you don't use static linking, so why add something that selects static libs over dynamic on windows and apple? Does it work without that? Did you just copy and paste it from somewhere else? We like to know why things were done, and I don't mean "It did't work, and now it does".

    Forgive what might come over as a rant, but the majority of these changes are totally unacceptable - even if just for the way they were presented.

  4. Paul Turner

    I briefly investigated the link library error, and discovered it's due to some horrid #pragma uses in CEGuiOgreBaseApplication.h that attempts to link those libs - I can only apologise for those not getting removed earlier!

  5. OgreT author
    • Also, you hack the cegui/cmake/FindOgre.cmake file - which is fair enough to change, btw - but then just leave it there hacked.

    You are right, it's better to delete the whole file. I changed it first and remove the call of it later without deleting it.

    • If Ogre requires boost libraries to be linked in then the Ogre CMake module should provide the names of those libs.

    Yes, this is a pain which is still not solved by the Ogre team, now.

    • Did you just copy and paste it from somewhere else?

    Should I post all commands and results in details? This is a lot of text!

    • You say you don't use static linking, so why add something that selects static libs over dynamic on windows and apple?
    • Why did you not refactor this into a single check? See to me it just looks like careless hacking.

    Let me explain my careless hacking in details for you:

    if (WIN32 OR APPLE)
        set(Boost_USE_STATIC_LIBS TRUE)
    else ()
        set(Boost_USE_STATIC_LIBS ${OGRE_STATIC})
    endif ()
    

    Irrespective of static or dynamic building CEGUI you can use dynamic or static boost libs. You can build static CEGUI with dynamic boost or dynamic CEGUI with static boost. There are some prefences for windows and apple to use the static boost libs, so there are no boost DLLs required.

    set(Boost_ADDITIONAL_VERSIONS "1.54" "1.54.0" "1.53" "1.53.0" "1.52" "1.52.0" "1.51" "1.51.0" "1.50" "1.50.0" "1.49" "1.49.0" "1.48" "1.48.0" "1.47" "1.47.0" "1.46" "1.46.0" "1.45" "1.45.0" "1.44" "1.44.0" "1.42" "1.42.0" "1.41.0" "1.41" "1.40.0" "1.40")
    

    You're searching for boost 1.36 at the moment in line 106:

    find_package(Boost 1.36.0 COMPONENTS python unit_test_framework system)
    

    The current versions are 1.53 (stable) and 1.54 (dev, RC). To use up to date boost versions you have to remove the 1.36.0 part of your line. You should also add Boost_ADDITIONAL_VERSIONS because it's independent of CMake version now. Every CMake version has a build in Boost_ADDITIONAL_VERSIONS with the available boost versions at the moment of the release.

    if(OGRE_FOUND)
        set(CEGUI_BOOST_COMPONENTS python unit_test_framework system thread date_time)
    else()
        set(CEGUI_BOOST_COMPONENTS python unit_test_framework system)
    endif()
    find_package(Boost COMPONENTS ${CEGUI_BOOST_COMPONENTS} QUIET)
    

    First search: If we use Ogre we have to search for thread and date_time, too.

    if (NOT Boost_FOUND)
        # Try again with the other type of libs
        if(Boost_USE_STATIC_LIBS)
            set(Boost_USE_STATIC_LIBS OFF)
        else()
            set(Boost_USE_STATIC_LIBS ON)
        endif()
        find_package(Boost COMPONENTS ${CEGUI_BOOST_COMPONENTS} QUIET)
    endif()
    

    Second search: If it's not possible to detect the first boost configuration we change it and search aggain: static boost -> dynamic boost or dynamic boost -> static boost.

    if(Boost_FOUND AND Boost_VERSION GREATER 104900 AND OGRE_FOUND)
        set(CEGUI_BOOST_COMPONENTS python unit_test_framework system thread date_time chrono)
        find_package(Boost COMPONENTS ${CEGUI_BOOST_COMPONENTS} QUIET)
    endif()
    

    Third search: If we use Ogre and we have boost 1.5x, we also have to search for chrono which is a dependency of boost date time since 1.50. It's not possible to combine this before, because we don't know the boost version.

    Btw, even If you don't like Ogre you should update your boost search from 1.36 to 1.5x.

    I can only apologise for those not getting removed earlier!

    I can only apologies for reporting this error that late.

  6. Paul Turner

    the 1.36.0 is the minimum required - it will find higher versions, as specified in the documentation for the FindBoost.cmake, so there is no need for that to be removed at all.

    Also, it's not a case of whether I like Ogre or not. I think Ogre is, generally, fine. Yes, things have deteriorated quite a bit in more recent times, and this whole boost debacle is a massive stain on what was once a really, really great library. It is up to the Ogre devs to provide at least a part-way solution to that by giving some output from their FindOgre module as to what boost libraries are required - based on options specified when the specific version of Ogre was built. For others to have to guess or assume about that is nothing short of terrible (what if Ogre was not built using boost threads? We still bring that lib in? That might not be a big deal on Windows, on other systems the reference to the lib is still made and it still gets attached to the output binary regardless of whether it's actually needed, which really sucks). So, on this matter, I take a stand, because while ever everyone else is trying to work around issues not being addressed by the Ogre devs, the Ogre devs never will address them (they probably will not anyway, but I will not take part in encouraging their apathy towards these types of issue).

    With regards to static vs dynamic linking of libs, I'm well aware that you can link static libs in dynamic configuration - it creates a lot of potential issues (multiple copies of symbols in separate modules and such), but yes, some people prefer to do that, I agree. So should it not be up to the user to choose which they prefer? Rather than automatically preferring static on windows and apple? I use an Apple Mac, and I for one would never choose static as the default. And on non windows or apple systems the preference for static vs dynamic boost comes is decided based on whether Ogre is static? Why should that be the case?

    Anyway, we have tickets opened for the Overlay and CEGUI_DETELE_AO build issues (can't cherry pick from your commits, because there was only one commit - that's why we like fine grained commits, so that even if we have to decline a pull request, we can still make use of parts that are decent) and also for that #pragma linking issue - these will be fixed for 0.8.2 which will release next week.

    I'm going to be done with this now.

  7. OgreT author

    can't cherry pick from your commits, because there was only one commit - that's why we like fine grained commits

    Can you tell me how to create multiple pull request on bitbucket? bitbucket is always merging multiple changes to one pull request. I had this discussion at ogre before - without a usable solution.

  8. Martin Preisler

    Hi transporter, missed this question. You can have multiple pull requests if you have multiple branches. All the branches can target a single branch in the upstream repository.