MWRender Refactoring

Everything about development and the OpenMW source code.
User avatar
Zini
Posts: 5537
Joined: 06 Aug 2011, 15:16

Re: MWRender Refactoring

Post by Zini » 12 Nov 2011, 20:41

Only in mwclass/*.cpp please. Those people with slower computers will thank you for it (avoiding includes in hpp files where possible can improve compile time a lot).

jhooks1
Posts: 780
Joined: 06 Aug 2011, 21:34

Re: MWRender Refactoring

Post by jhooks1 » 12 Nov 2011, 21:59

Committed corrections.

User avatar
Zini
Posts: 5537
Joined: 06 Aug 2011, 15:16

Re: MWRender Refactoring

Post by Zini » 12 Nov 2011, 22:20

Almost:

Including objects.hpp in mwclass/creature.cpp and mwclass/npc.cpp is pointless, since you won't be using it.

You added insert functions for itemlevlist.hpp. But this class is currently not rendered (we still need to figure out how to handle it). Also, adding the function only to the hpp file, will result in a link time error. Best remove them again for now.

The include guards are mostly okay. We use a slightly different naming scheme (compare it with other files). Also the underscore at the beginning is theoretically not allowed in C++ (reserved name), though I don't know of any compiler that would reject it. You can leave the new include guards unchanged, but please keep these points in mind when writing new files again.

The renderinginterface.hpp include in renderingmanager.cpp is redundant, since you are including this file already in the header.

jhooks1
Posts: 780
Joined: 06 Aug 2011, 21:34

Re: MWRender Refactoring

Post by jhooks1 » 14 Nov 2011, 00:20

Committed corrections.

jhooks1
Posts: 780
Joined: 06 Aug 2011, 21:34

Re: MWRender Refactoring

Post by jhooks1 » 14 Nov 2011, 05:20

Changed a lot of things in scene and world. Ignore the backup files, committed them on accident.
Please let me know of anything that is wrong. There are 3 new commits.

User avatar
Zini
Posts: 5537
Joined: 06 Aug 2011, 15:16

Re: MWRender Refactoring

Post by Zini » 14 Nov 2011, 10:16

That was a pretty large commit. Not sure if I found all problems. But this is what I see:

1. the loadCell function is incomplete. You still need to iterate over all MW-references in the cell and call RenderingManager::addObject on it.

2. As far as I can see the mwRoot argument in the Scene's constructor is unused.

3. The enable and disable functions won't work. You don't actually enable/disable anything in the rendering system. Tricky. It seems we overlooked this feature and since I am planning to do some changes to the non-rendering part of enabling/disabling after you are finished with mwrender, anything we adjust now will only cause problems later.
I suggest you add a doxygen To-Do comment in both functions (/// \todo enable/disable reference in the OGRE scene). I will then take care of it later.

4. All those looping constructs are unfortunate. This kinda of repetition reduces the maintainability of the code and also these loops express the intend of the code badly. Maybe it would have been better to use a std::set instead of a std::list and utilise the build-in ordering to check if a cell is listed.

5. The removeObject code is wrong. Shouldn't even compile. I repeat, the RenderingInterface is meant for use by the MWClass-hierarchy. Do not try to make use of it from somewhere else. Please use the RenderingManager instead.

6. You still have plenty of out-commented code in the source. Please make sure you remove it.

jhooks1
Posts: 780
Joined: 06 Aug 2011, 21:34

Re: MWRender Refactoring

Post by jhooks1 » 14 Nov 2011, 19:20

Zini wrote:That was a pretty large commit. Not sure if I found all problems. But this is what I see:

1. the loadCell function is incomplete. You still need to iterate over all MW-references in the cell and call RenderingManager::addObject on it.
So make new insertCell() and insertCellRefList() functions like the ones in cellimp?

Zini wrote:
5. The removeObject code is wrong. Shouldn't even compile. I repeat, the RenderingInterface is meant for use by the MWClass-hierarchy. Do not try to make use of it from somewhere else. Please use the RenderingManager instead.
Do you mean PhysicsSystem::removeObject() or World::deleteObject() or Object::deleteObject()? I didn't use the RenderingInterface in any of these.

User avatar
Zini
Posts: 5537
Joined: 06 Aug 2011, 15:16

Re: MWRender Refactoring

Post by Zini » 14 Nov 2011, 19:35

So make new insertCell() and insertCellRefList() functions like the ones in cellimp?
Yes. The signatures will probably change a bit, but otherwise you can copy and paste these functions.
Do you mean PhysicsSystem::removeObject() or World::deleteObject() or Object::deleteObject()? I didn't use the RenderingInterface in any of these.
I see. Okay. The call to PhysicsSystem::removeObject is correct, but you still need to remove the object from the RenderingManager.

jhooks1
Posts: 780
Joined: 06 Aug 2011, 21:34

Re: MWRender Refactoring

Post by jhooks1 » 16 Nov 2011, 05:32

I am getting this error with my latest commit:

Error 28 error C2243: 'type cast' : conversion from 'MWRender::RenderingManager *' to 'MWRender::RenderingInterface &' exists, but is inaccessible C:\c\openmw\apps\openmw\mwworld\scene.cpp 63

The only way I know of to fix this is public inheritance from RenderingInterface, it was specified earlier in this topic that it needed to be private.

User avatar
Zini
Posts: 5537
Joined: 06 Aug 2011, 15:16

Re: MWRender Refactoring

Post by Zini » 16 Nov 2011, 09:24

Okay. I repeat again (and this time rightfully and not because I misread your code ;) ): Do not use the RenderingInterface outside of the MWClass hierarchy. Rendering requests should be run through the RenderingManager (RenderingManager::addObject).

Post Reply