[Obsolete] Improvements and fixes to OgreMain, Direct3D9, Direct3D11 and the RTSS.

#555 Declined
Repository
LiorL
Branch
default
Repository
sinbad
Branch
default
Author
  1. God
Reviewers
Description

This PR has been split into PR's #577, #579 #588 and #609

Comments (45)

  1. Pavel Rojtberg

    I agree that this should be split up by severity of change and by module. At least separate the RTSS/ Direct3D9/ Direct3D11 parts into separate commits. Also split fixes from new functionality - especially in cases when you change core functionality like the hash function.

  2. Matias Goldberg

    I agree with Jannik too on all of his points.

    There is some good stuff here, but all intermixed. Somewhat related, I'm going to hate merging this with 2.1; namely because:

    • DepthBuffers have changed a lot. In 2.1 we added support for depth textures, and we handle D3D11 creation differently.
    • D3D11HardwarePixelBuffer was extremely broken. We fixed a ton of leaks, out of memory errors, texture array issues, cubemap issues, vendor-specific bugs (particularly some NVIDIA models, e.g. one of their models likes to queue texture uploads until it runs out of memory instead of flushing periodically)
    • D3D11 doesn't need state cache saving hacks
    1. God author

      D3D11 doesn't need state cache saving hacks

      We gained 8-13 percent in FPS using the save state mechanism. So, what do you exactly mean ?

      1. Matias Goldberg

        It's in relation to merging 2.1 I have no doubt it improved performance in the 1.x branch. Thing is, it feels like an intrusive hack (and I'll also have to undo it when merging to 2.1).

        1. God author

          It would probably improve performance in 2.1 as well, too bad loosing it, especially when it's conditioned with a compile constant. Perhaps a less hacky implementation would be satisfying enough for it to be integrated into 2.1.

          1. Matias Goldberg

            For a state cache to work well, there has to be enough redundant changes in the first place. Ogre 2.1 barely performs any redundant state change because it was optimized and refactored at a higher level to avoid the situation. For instance, the rasterizer and blendstates are part of the material. We just provide them and set them directly when it needs to change.

            Ogre 1.x instead just recreates the D3D11 API objects or caches them at RS level based on the material parameters, every time. A state tracker at the low level would only hurt performance checking if the state needs to be changed only to find out almost always it needs to. There's a reason Ogre 2.1 yields between 3x-10x improvements on real world scenes over 1.x :)

            This has already happened with GLES2, where the state tracker showed significant performance improvements on 1.x branch; but decreased it on 2.1 (that was before we broke GLES2 support; right now it's not working)

  3. God author

    While I agree that could be useful, we have to think about consistency here. If there is a constructor for a Vector3 from a Vector4, then we should also have a constructor for a Vector2 from Vector4, and Vector2 from Vector3.

    I agree, it would be a better practice.

    Added: named viewports

    The goal is to identify a unique viewport, it may prove useful for future changes in Ogre, and immediate benefits to the end user. alternatively we may do as you suggest and add UserObjectBindings to Viewport, I'm not so sure what's better.

    Added: 'Enabled' property to Viewport

    It's related to uncommitted changes for a feature called "Instanced Viewport" which should have been included in this pull request, but I thought it's already too much.

    I believe that change breaks older VS compilers, because MS doesn't care about implementing C99.

    MS didn't care, now they do. it'll be fixed for VS2008 and earlier.

  4. God author

    Guys, please correct me if I am wrong, but I got the impression that the pull request is being addressed as a binary change. meaning, before and after. Perhaps that's the reason why it looks monstrous. I tried my best to keep the Changesets as minimal as possible so reviewing it would be easy and easy to track.

    So in essence the pull request is indeed divided, by the changesets.

  5. Assaf Raman

    dark_sylinc - I think that the merge to OGRE 2.x shouldn't be a big consideration, as it seems that many changes are not being merged back to 1.x and so there is already some kind of code split.

    Lior - Regarding stdint.h - we can wait with this change as it doesn't seems like a must, can you remove them?

    Lior - can you explain more why not to use UserObjectBindings?

    Lior - Regarding the Vector4 constructor and so on - can you add constructors to the rest of the vector classes?

    Pavel Rojtberg - I don't see how he can split so many topics up, he did split the commits into topics and we have done such merges in the past and it was always to the good of the project.

    In general regarding this pull request - this is almost a year of effort of fixing OGRE and adding new features to it, just run the samples in DirectX 11 using this code and compare it to the existing code and see for yourself the benefits. Some of the benefits are for more then one render system. The merge to OGRE 2.x may be hard but a big part of the community still work with OGRE 1.x as OGRE 2.x is not finished yet and will benefit from the changes. I vote to merge in any case, if you review and wants Lior to to fix anything before or after - I am sure he will be happy to do so.

    Lets give our comments, Lior will do his best to make us happy - then merge the code into our repo.

    1. God author

      Regarding stdint.h - we can wait with this change as it doesn't seems like a must, can you remove them?

      Done

      Regarding the Vector4 constructor and so on - can you add constructors to the rest of the vector classes?

      Done

  6. Pavel Rojtberg

    I think that merging the changes into 2.1 should be considered. The split is bad as it is, but losing any progress made in the 1.x branch would be even worse. Ideally getting the changes from 1.x to 2.1 should be just one hg merge. However one could let individual components like the DX11 RS diverge as they are very different between 1.x and 2.1 anyway.

    I am also strongly against this bulk-style merges. Pull requests should be focused and not touch unrelated files - however this is inevitable with the bulk style. Last time you did the bulk merge the SSAO Sample broke with GL because some commented out lines slipped through.

    Using the histedit extension (git interactive rebase) to split the history into individual branches is also not much work.

  7. God author

    Assaf, in response to

    can you explain more why not to use UserObjectBindings

    UserObjectBindings is for genral purpose usage.
    I think that a more specific solution would be better, such as adding a unique identifier for the lifetime of the application to a viewport, henceforth, "Named viewports" (changeset 4411d1ee6cd6) .

    Currently a unique viewport can not be easily identified, if at all.
    We can't rely on it's address in memory, since the CRT sometimes return the same memory address.
    Named viewports allow us to track creation and destruction of a unique viewport across the lifetime of the running application.
    this is most helpful when debugging complex issues, or when creating and destroying viewports fast enough due to end user misuse.
    It's easier when debugging in general and it eliminates misuses of current developers that use the Viewport address in an unsafe manner, such as

    void replaceIfChanged(Viewport* viewport)
    

    I believe that the main benefits in general are:
    1. safety and fool proof.
    2. easier maintenance.
    3. zero risks.

  8. Assaf Raman

    It is always better to have small commits and pull requests; sadly it is not always possible. This is this case, Lior's is on a different source control system and the merge needs to be done by hand every time the company he works for releases a version – after the testing its end product that includes the code. This merge effort take weeks because of the effort to do small commits on a by topic basis. To have it as many pull request is almost impossible. So we have the option not to merge the code, in the case I believe it is unlikely we will get any more pull requests from his company, or we can merge. Regarding your comment about me writing "we have done such merges in the past and it was always to the good of the project" – well, if we got a big pull request from a good source and in the past issues were minimal – I think it is a thing worth knowing. I think we should merge considering what I wrote here, it is very rear for companies to give their modified code back to OGRE and this is code that was developed and released as part of a real product and not just for the fun of it.

  9. Matias Goldberg

    The situation is not ideal, but it's not terrible either, mostly because I trust Lior making high quality changes. Though it would be nice to have something smaller to review.

    Mercurial allows for merging in incremental steps (e.g. merge revision 10, then merge revision 20, then merge revision 30; creating a total of 3 commits) but it seems Bitbucket pull request system doesn't provide this functionality.

    I guess I'll have to suck it up, and do more work when merging 1.10 -> 2.1, or perform grafts for individual & useful bugfixes that I see fit.

  10. God author

    This I do not see as valid arguments. With the same logic we should be adding unique names to every object that OGRE creates, ever.

    Absolutely true, this is not a satisfying reason to add this change.
    I was biased by the clean solution it gave me for my specific use case while it is apparent to contribute nothing to Ogre.
    I originally thought of it as a helper routine for the end user, which is less cumbersome than UserObjectBindings, but less correct as you pointed out.

    I may deduce from your reply that you may not necessarily disagree with my arguments in general, regardless of the "Named Viewports".
    Perhaps we do should add meta-data to Ogre's objects, such as unique identifier, timestamp, etc. , has it been discussed ?

  11. Pavel Rojtberg

    I do not think the decision should be based on whether one trusts Lior to make high quality changes, but on whether the changes are indeed high quality. I mean this is the sole point of peer-review (the peer part implies some degree of trust). Generally we want to merge the good parts while backing out the bad ones. Bad in this context means controversial i.e. where not everyone agrees that they are necessary. As we use bitbucket we are kind of bound to the workflows it imposes. Creating one pull request therefore means take all or take nothing.

    If cherry-picking with mercurial is easy, then Lior can easily create new branches and pick the corresponding changes onto them to split this PR up.

    If changes to Ogre are tracked in a different source control system at Liors company, then this is an argument for fixing their workflow and not for accepting huge PRs. We can still talk about how to make Ogre easier to integrate for Liors company. (git submodules anyone?)

    1. Assaf Raman

      You can always back out or change anything that you don't like later on, in the end this is only code that can always be modified. Nothing is set in stone.

  12. Pavel Rojtberg

    finally got it to compile: this breaks pretty much all of the samples with GL3Plus/ GLX.

  13. Eugene Golushkov

    ID3D11DeviceContext leak is introduced in D3D11RenderWindowSwapChainBased::_resizeSwapChainBuffers

  14. Eugene Golushkov

    mWindowHierarchy is initialized once and never updated, and is used only to traverse hierarchy up to test IsIconic on it inside D3D11RenderWindowHwnd::isVisible(). It is better to remove this collection and traverse window parents directly each time D3D11RenderWindowHwnd::isVisible() is called as traversal is very efficient, and in this case we could no worry about keeping collection up to date. The same is true for D3D9 RS. And may be IsWindowVisible should be called too

  15. Eugene Golushkov

    mAlwayWindowedMode should be spelled as mAlwaysWindowedMode and probably cause DXGI_MWA_NO_ALT_ENTER flag to be passed to mpDXGIFactory->MakeWindowAssociation(mHWnd, <flags>)

  16. Eugene Golushkov

    Adding virtual destructor to Ogre::Plane is like adding virtual destructor to Ogre::Vector3. It is primitive value type, used primarily for intersection tests, there is nothing to destruct, it should not have vtbl.

  17. Eugene Golushkov

    enum Capabilities : capsType ... - it`s C++11, would not be compiled by C++98 compiler

    the same for enum GpuParamVariability : uint16,

    1. God author

      This change fixes RSC_COMPLETE_TEXTURE_BINDING capability.
      One option is to readjust the groups/capabilities.
      The most convenient option IMHO is to force a 64 bit enum by adding Undefined = ~0x0ULL (Large number) to the enum
      as discussed here http://stackoverflow.com/questions/76624/64-bit-enum-in-c.
      I'm not sure how different compilers will behave, though it seems like a 64 bit integer support is expected from the compiler by Ogre.

      1. Eugene Golushkov

        ULL suffix as well as long long type is C99 and C++11. It is supported by really old versions of gcc as part of C99 support, but for VS2008 you should use __int64 datatype and i64 suffix. I really don`t know if VS2008 should be supported by Ogre. At least, long long is supported much more often than enums with base type.

        1. God author

          I had some issues making it work properly using the current code, eventually I solved it by reverting back the code and changing group for RSC_COMPLETE_TEXTURE_BINDING

  18. Eugene Golushkov

    inline explicit Vector2(V3&) - this constructor would not be inlined - body is in OgreVector2.cpp file. Moreover, this body is not used while compiling that cpp file and could be even optimized out, causing linker error about missing function body if it is used by some other translation unit. Exactly this situation is described here http://blogs.msdn.com/b/vcblog/archive/2014/09/26/feedback-making-zc-inline-default-for-debug-release-configs-in-14.aspx

    The same is true for inline explicit Vector3(const Vector4& vec4); My suggestion - these value types should have header only implementation, that should be inlined by compiler.

    1. God author

      Fixed in Changeset 9f6f36319a85.
      The only non-inlined method is

      explicit Vector3::Vector3(const Vector4& vec4)
      

      due to circular reference with Vector4.h

  19. Eugene Golushkov

    FastHash function is used inside StreamSerialiser::calculateChecksum, and the result is written to the stream. Probably you should not replace algorithm used inside FastHash, or you broke backward compatibility with data serialized by older Ogre version.

  20. Eugene Golushkov

    Combining Frustum::enableReflection(const Plane& p) and enableReflection(const MovablePlane * p) into the one setReflectionPlane(const Plane * p) only to immediately split code based on RTTI check dynamic_cast<const MovablePlane*>(p) - IMHO it`s not good. BTW it additionally requires Plane to be polymorphic, that is especially bad.

    1. God author

      this discusses also the comment https://bitbucket.org/sinbad/ogre/pull-requests/555/improvements-and-fixes-to-ogremain/activity#comment-9396720.

      In this change I intended to simplify usage and maintainability.

      • Plane is not "final" class as Vector3 is, Plane also functions as a base class for MoveablePlane.
      • This change removes duplicity, where the same common operations are taken place for Plane and MoveablePlane.
      • I find the API simpler to use, at least in my use case.

      it additionally requires Plane to be polymorphic, that is especially bad.

      Bad in what way ? performance ?

  21. Eugene Golushkov

    Why did you defined GpuProgramType& operator++(GpuProgramType &c) over the enum? Seems that it is not used in Ogre, and can hide unintentional programmer mistake. It`s saves only one cast in enumeration like "for(GpuProgramType t = GPT_FIRST; t < GPT_LAST; t = GpuProgramType(t+1))" but it is not inlined and adds unnecessary exported symbol.

    1. God author

      The idea is to save the exact cast you've mentioned, for a cleaner implementation.
      One usage example may be found in HLSLProgramProcessor::postCreateGpuPrograms.

      can hide unintentional programmer mistake

      In what way ?

  22. Matias Goldberg

    I'd like to merge PRs #559 & #561 before merging this one to make merging with 2.1 easier (since several of his contributions are useful there, whereas in this commit there will be many things I'll have to reject/revert because it just works very differently now)

    I also assume merging that PR will probably cause some merge conflict with this PR. Same doing viceversa (merging this PR first). Any comments? @eugene_gff @LiorL @assaframan ?

    1. Eugene Golushkov

      I looked at changes, seems that actual amount of conflicts would be not so big, as multidevice support seems to be very independed with a few #ifdef-ed integration points.

  23. Eugene Golushkov

    There are now several commits that are reversed by other commits. It could add unnecessary complexity during future cross-branch merging, if such commit history would be accepted into Ogre as is. I propose to use mq to reorder such patches together and further fold them into the one and further eliminate such combined patches if it will consist from whitespaces only. As we have at least v1.10, v2.0 and v2.1 branches now - it better to do this outside of Ogre repository, and merge already simplified pull request. Additionally, it may be useful to reorder non-controversial patches to be the first, controversial in middle and very project specific to be the last - we did this at our company, so that declined pull requests like #364, #277 just became a private, and are rebased on top each time we updating Ogre to fresh revision.