MWRender Refactoring

Everything about development and the OpenMW source code.
Locked
User avatar
lgromanowski
Site Admin
Posts: 1193
Joined: 05 Aug 2011, 22:21
Location: Wroclaw, Poland
Contact:

MWRender Refactoring

Post by lgromanowski »

Zini wrote: Here is a first sketch of my proposed new design:


General structure:

Most mw*-subsystems have one top-level facade-like class. mwrender doesn't and that has always been a big part of the problem. I propose the following design:

1. A top level facade class: MWRender::RenderingManager

This would be the only class the outside of MWRender would interact with.

2. Middle-layer-classes

One for every rendering-aspect. Will be used by the RenderingManager

3. A bottom level class, that manages the OGRE-interaction: MWRender::Scene

Used by the middle-layer-classes.


Physics clean up:

Currently we have the physics code in the MWScene class. That kinda made sense in the original context, but with the new design we need to remove the physics from mwrender. I suggest we move the physics system to MWWorld and give MWWorld::World an instance of it. Object-related calls to the renderer would then have to be accompanied by similar calls to the physics.
Also I suggest to change the way how the physics are interacting with the world. Currently during physics calculations various calls to MWWorld::World are made. It would be better to have the phyiscs system generate a vector full of handle/coordinate pairs. MWWorld::World::doPhysics would get this vector from calling the physics system and would then modify the world accordingly. This means the new physics system will not depend on MWWorld::World anymore.


Changes to MWWOrld::World:

The interface should not change.

World will get an instance of the RenderingManager (via pointer), that it should not expose to the outside world. However it will at various points pass this RenderingManager as an argument to various
MWWorld::Class functions.

mBufferedCells has to go (we don't support this concept anymore). mActiveCells must be changed from a map to a set (since MWRender::CellRenderer has to go too).
Currently MWWorld::World has a pointer to the SkyManager. That must go too. Calls to the SkyManager should be directed through the RenderingManager.


The MWRender::RenderingManager class:

- manages instances of the Scene class and the middle-layer classes
- should not do anything but forward calls to its members.


The MWRender::Scene class:

- interface to OEngine::Render::OgreRnederer
- needs to handle the root node (see old MWScene class)

Alternatively We may decide to extend OEngine::Render::OgreRenderer instead. This would make the Scene class redundant.


The MWRender::Objects class (middle-layer):

- object management
- batching
- must to be able to put a whole cell into a scene efficiently (and also remove it)


The MWRender::Actors (middle-layer).

Rendering creatures and NPCs is completely different from rendering ordinary objects and might be best separated into its own class.
Actually I am not 100% sure about the creatures. Maybe they would be best moved to the ordinary objects. Input welcome.


The MWRender::Player class (middle-layer):

- The MWRender::RenderingManager class should filter out Player-related calls and redirect them to the MWRender::Player instance.
- handles camera modes and camera positioning (we don't need to implement them during the refactoring; this is a separate task)
- in first person mode, the player scene node should simply be made invisible
- may share code with MWRender::Actors through a common base class or a shared helper class


The MWRender::Sky class (middle-layer):

- old sky manager
- drop the PIMPL idiom (not needed here and inconsistent with the rest of OpenMW)


Possible future classes: MWRender::Water, MWRender::Terrain (middle-layer)
Star-Demon wrote: Where would we be able to fit HDR-like extensions to the renderer?

MWRender::HDR?

As well, if we wanted to later extend the lighting functionality - would we be altering MWRender::Lights or would we be going deeper?

Otherwise this looks very organized. I imagine it looks more like a balanced tree now.
Zini wrote: HDR is pure compositor/shader stuff. Probably OpenEngine, maybe with modifications to the NIF-importing components.
As well, if we wanted to later extend the lighting functionality
That entirely depends on what you have in mind regarding lighting.
Star-Demon wrote: Specifically, newer kinds of lighting (other than what I see ogre supports) and shadows.
Zini wrote:
newer kinds of lighting (other than what I see ogre supports)
Again to unspecific.
shadows
That is an Ogre core feature. We don't need any substantial amount of code in our render system to make use of shadows. And what little we need (configuration) probably goes best into OpenEngine.

btw. I am nearly totally convinced now that we integrate the originally planned Scene class into OpenEngine.
Star-Demon wrote: The Scene class we were going to write? Or the one you wanted to inherit from?

In any case - the shadow support in ogre is good. I suppose I'm more interested in how we'll use our scenemanager to make/fake HDR later on.
Zini wrote:
The Scene class we were going to write? Or the one you wanted to inherit from?
There is only one scene class in the whole proposal. And no inheritance at all.
Zini wrote: Two more postings and I am done.

Here is a revised list of middle-layer classes:

- MWRender::Objects (as before)
- MWRneder::Actors (as before, split of NPCs and Creatures subject to further discussion)
- MWRender::Player (as before)
- MWRender::Sky (as before)
- MWRender::Water (later)
- MWRender::Terrain (later)
- MWRender::Debugging (various pieces of special rendering, that is used during content debugging; e.g. physics collision shapes)

For the RenderingManager we need the following interface:

- addCell (CellStore *store);
- removeCell (CellStore *store);

These would basically iterate over all objects in a cell and add/remove them (see below). But RenderingManager must also keep track of which cells it is currently supposed to render (also see below).
When objects are added they also need to be checked, if they are disabled or deleted (which means they should be skipped).

- addObject (const MWWorld::Ptr& ptr, CellStore *store);
- removeObject (const MWWorld::Ptr& ptr, CellStore *store);

Adds/removes object to/from the scene. If debugging rendering is enabled, this must also be added for the object (in the addObject case).

- moveObject (const MWWorld::Ptr& ptr, const Ogre::Vector3& position);
- scaleObject (const MWWorld::Ptr& ptr, const Ogre::Vector3& scale);
- rotateObject (const MWWorld::Ptr& ptr, const::Ogre::Quaternion& orientation);
- moveObjectToCell (const MWWorld::Ptr& ptr, const Ogre::Vector3& position, CellStore *store);

Should be obvious. moveObjectToCell does the same as move, but also need to move the scene node to a different cell base scene node.

- setPhysicsDebugRendering (bool);
- getPhysicsDebugRendering() const;

Needs to adjust an internal flag, that is considered when addObject/removeObject is called. Must also add/remove collision box rendering for cells currently registered as rendered (see addCell/removeCell function).

- update (float duration);

Called from Engine-framelistener (via MWWorld::World). Apparently required by animation rendering.

- some functions for controlling player camera mode

To be determined.

- all of the sky-related functions (to lazy to list them, just copy them from the current sky-manager class)
Zini wrote: And the last one:

Currently all object rendering goes through MWWorld::Class::insertObject. We should keep this pattern, but I think I need to explain it a bit more.

This function was never meant to get any rendering code. All the rendering code should have gone into MWRender. insertObject should have used basic building blocks for object rendering (provided by MWRender), like meshes and light. That worked okay for regular objects, but it caused a load of trouble for creatures/NPCs.

I propose the following change: We make creatures/NPCs into one of these basic building blocks. After creating the scene node, insertObject calls a single function somewhere in MWRender, that attaches a whole animated creature to the node.

Currently the interface for these basic building blocks is in CellRenderImp. We should keep the overall structure of the offered functions (minus all the bloat that showed up during NPC rendering). But the class itself has to go (makes no sense anymore).

It would be best to add these functions to a new abstract base class (MWRender::RenderingInterface) from which MWRender::Objects, MWRneder::Actors and MWRender::Player are derived. In the signature of the insertObj function this base class would replace CellRenderImp.
Depending on the type of object MWRender::RenderingManager::addObject can then offer insertObj the appropriate rendering instance.
Zini wrote: Oh, one more:

What I haven't worked out yet is how to handle the player and NPC/creature orientation (required by physics code to determine the direction of the force vector). What we currently have is a nasty hack and I think it should go as a part of the refactoring process. But I don't have an idea how to do it yet.


Okay, that is all for now.

@gus: Can you work with this concept?
Zini wrote: Actually, here comes an idea for Player/NPC/Creature orientation. Currently MWorld::Class offers a desired movement vector.

We can give the MWRender::RenderingManager class a getPlayerOrientation function that retrieves the orientation based on the camera mode). This orientation would then be used once per frame to determine the desired movement vector and store it in the player object, where it can be read from the physics system like for any other NPC/Creature too.

btw. I know physics prefers quaternions, but for the NPCs/Creatures are vector makes more sense.
Star-Demon wrote: Vectors *are* more intelligible than quaternions, but the quaternions are more proper representations for movement - maybe between more objects than individual...

Wouldn't a vector require more updating for physics than a quaternion each frame?

I'm thinking that when many objects interact, you would be performing operations on quaternions and resolving a solution for a single object - wouldn't we have to convert a vector each time this would need to be done? - and if all were vectors- wouldn't these conversions be more difficult?
Zini wrote:
but the quaternions are more proper representations for movement
Actually they are overkill, because they also represent an orientation, which we don't need to determine the moving direction.
Wouldn't a vector require more updating for physics than a quaternion each frame?
No! Why should it?

Sorry, beyond that I haven't a clue of what you mean with this posting.
gus wrote: If i understand well, the purpose of the scene class is to "hide" Ogre from the middle layer class right?
But most of the rendering classes will need a direct aces to Ogre. So "hiding" Ogre from the middle layer class seems impossible.
So I don't really see the point of the scene class.

Star: vectors are what bullet wants.

I will look into this deeper in the next few days, and if i have any question, i will let you know ;)
Zini wrote:
If i understand well, the purpose of the scene class is to "hide" Ogre from the middle layer class right?
I think that was the original idea, when Nico started working on the rendering system. But nothing came from it and we can as well forget about it. So lets just use the OpenEngine class instead of a new scene class.
vectors are what bullet wants.
Funny! For some reasons I thought you said it takes quaternions.
gus wrote:
Funny! For some reasons I thought you said it takes quaternions.
They were needed on my original implementation, as the direction vector was in object space instead of world space.
But as long as the World class give the right direction vector(in world space), the orientation is not needed.
Also, as the shape is a capsule, the orientation doesn't matters.
I think that was the original idea, when Nico started working on the rendering system. But nothing came from it and we can as well forget about it. So lets just use the OpenEngine class instead of a new scene class.
Ok
Zini wrote:
Also, as the shape is a capsule, the orientation doesn't matters.
Just to avoid further confusion: The movement direction is not equal to the direction a character is facing. Are we clear on that? Your statement above makes me think that maybe we are not.
gus wrote: My bad. I was mixing stuff. We are clear on that.
But anyway, as the rotation is already stored with Ogre, no need to store it in bullet.

Should I update my git repo before starting the real work? (right now, i can't, because of my connection).
Zini wrote:
But anyway, as the rotation is already stored with Ogre, no need to store it in bullet.
Actually the rotation is stored in the world-model. What is stored in Ogre is just a copy.
Should I update my git repo before starting the real work? (right now, i can't, because of my connection).
Yes, please. Merge in my next branch. That is the most up-to-date line of development.
Star-Demon wrote:
Zini wrote:
but the quaternions are more proper representations for movement
Actually they are overkill, because they also represent an orientation, which we don't need to determine the moving direction.
Wouldn't a vector require more updating for physics than a quaternion each frame?
No! Why should it?
Sorry, beyond that I haven't a clue of what you mean with this posting.
You know, I can't really think of a reason off hand - I was pondering which is more efficient given what bullet wants given what they contain.
Zini wrote: @gus: Don't want to put pressure on you, but any progress? My own task for 0.12.0 is depending on this one and we can't integrate jhooks' animation work either before this task is complete.
Zini wrote: btw. Since the animation system might be ready soon, we will need two more functions for the RenderingManager class (either no implementation for now or simply pass them on to the MWRender::Actors instance):

void playAnimation (Ptr ptr, const std::string& animation, bool finishCurrentAnimation, bool skipToLoop, int loopCount = 1):

Play animation

finishCurrentAnimation: finish current animation before starting the new one
skipToLoop: only play loop-part of animation
loopCount: -1: infinite

void skipAnimation (Ptr ptr):

Skip animation for one frame.


Once you are done with the MWRender interface, I will hook up these (via the World class) to new script instructions (PlayGroup, LoopGroup, SkipAnim), so the animation code can be properly tested.
gus wrote: Sorry, i'm a little late.

But to answer your question, yes i've made some progress: I've not coded much, but I think I've figured out how things will work.

I also have a question: the rendering stuffs that were in the MWWorld::Class will go right?
Zini wrote: Not really. Quoting myself:
Currently all object rendering goes through MWWorld::Class::insertObject. We should keep this pattern, but I think I need to explain it a bit more.

This function was never meant to get any rendering code. All the rendering code should have gone into MWRender. insertObject should have used basic building blocks for object rendering (provided by MWRender), like meshes and light. That worked okay for regular objects, but it caused a load of trouble for creatures/NPCs.
As described elsewhere the NPC animation stuff should be moved to MWRender, but the other MWWorld::Class subclasses should hardly change at all.
gus wrote: Ok.
jhooks1 wrote: For the new NPC/Creature class we need the following:

std::vector of NiKeyframeData, each NiKeyframedata corresponds to one bone. The nif loader creates this vector and we grab it with a get function. This does not need to be modifiable.

std::map of std::string to int start index. The strings are going to be the name of each animation. This will need to be grabbed from the NIFLoader.
We need another map like this for the stop index. Neither one needs to be modifiable. I haven't started reading the text data though, so I am not 100% sure that it should work this way.

std::vector<int> rindexI, the current rotation indices. Must be modifiable. Each index corresponds to a bone.
std::vector<int> tindexI, the current translation indices. Must be modifiable. Each index corresponds to a bone.

float time, the current animation time. Must be modifiable.

Ogre::Entity model. This is the entity that contains the skeletal animation.

The entity,indices, and time must be updated with the time that has past since the last frame.
Zini wrote: Well, these are all implementation details and you can take care of them yourself, once gus is done. The question was a single NPC/creature class or a separate class for NPCs and creatures each. From what I read one class would be enough?
jhooks1 wrote: If you mean the npc class in npc.hpp and the creature class in creature.hpp. I think they may be able to share the same class, but they do have different insertObj() functions.
Zini wrote: No, that is not what I mean. The new render system will get a separate class for each aspect to be rendered: sky, terrain, objects. One of these aspects are the NPCs and the creatures. And we need to decide, if the rendering code and the required additional data structures are similar enough that using one class for both is enough.
gus wrote: The insertObj function takes MWWorld::Environment& environment as a parameters.
I don't know what it is, but it doesn't seems related to rendering, and it doesn't seems to be used (at least in the few classes that i checked).
Should I simply remove it?
Zini wrote: MWWorld::Environment originated in the script system, but then took a more general function. Its basically a collection of pointers to all subsystems or in other words a central hub, through which each subsystem can access each other subsystem.

Currently the insertObj functions does three different things:
#1 insert the object into the OGRE scene
#2 insert the object into the physics system
#3 insert the object into whatever game-mechanics related subsystem that needs to know about it

For #3 we need the MWWorld::Environment parameter.

I am not entirely sure what the best course of action would be. At least when switching camera mode, some of these functions may be needed independently of each other, so it might make sense to split the insertObj function into three (of which only one has a MWWorld::Environment parameter).
gus wrote: Splitting make sens.
I propose 3 function:
-insertObjRender
-insertObjPhysic
-insertObjScript

But the same job would be needed for insertObjPhysic and insertObjScript than for insertObjRender (iterating every object of the cell).

Maybe it could be better if the iterating job was done by the World class for all these 3 function at the same time, than by the render system, then the physic system,etc. That make it easier to track what is created, and when, i think. Do you agree?
Zini wrote:
insertObjScript
Make that insertObjMechanics or something. There isn't really any scripting involved.
Maybe it could be better if the iterating job was done by the World class for all these 3 function at the same time, than by the render system, then the physic system,etc. That make it easier to track what is created, and when, i think. Do you agree?
That is a bit tricky. I agree that having 3 separate loops would be wasteful. But I don't think the world class is the right place for a unified loop. The world class is about the world, while MWRender is only about what is currently in the scene (which is exactly what we need to loop over).

This is just an idea, but can we move it into MWRender::RenderingManager? This would require making the new physics system known to MWRender, but only by forward declaration, so we aren't creating any new dependencies here.
gus wrote:
This is just an idea, but can we move it into MWRender::RenderingManager? This would require making the new physics system known to MWRender, but only by forward declaration, so we aren't creating any new dependencies here.
It would make things harder to understand/debug. IMO, MWRender should only do Render, and nothing else.

Maybe we should make a new class in MWWorld which deals only with what is currently in the scene?
Zini wrote: I am not sure how that would look. But feel free to sketch some interfaces.
Zini wrote: Okay, thought about it a bit more. Makes sense. I suggest a single class MWWorld::Scene, of which MWWorld::World has a member that is not exposed to the outside. At some point we even might move some other parts of MWWorld::World into Scene that are not rendering/physics related. But that can wait.
gus wrote: I'm sorry, but I've realized i don't have enough time to complete the task, at least for the next few weeks. So it might be better to put someone else on this task, or report it.

I know I'm letting you down, but I prefer to say "i can't do this" than try to do it, and slow everything down.
Zini wrote: Okay. No problem. Shit happens.

Any volunteers?

Edit: Do you have at least enough time to have a go at: http://bugs.openmw.org/issues/154 ?

This is an important task and I don't think anyone else will pick it up any time soon.
swick wrote: So, we want insertObjRender, insertObjPhysic and insertObjMechanics on MWWorld::Class and MWWorld::Scene which calls them or do we want MWWorld::Scene to do the job of the 3 methods?
Zini wrote:
So, we want insertObjRender, insertObjPhysic and insertObjMechanics on MWWorld::Class
Yes. With an empty default implementation for insertObjMechanics and the two others as pure virtual functions.
[we want insertObjRender, insertObjPhysic and insertObjMechanics on] MWWorld::Scene
No. When objects or cells are added MWWorld::Scene is responsible for calling these functions. Actually only insertObjPhysic and insertObjMechanics. The requirement for calling insertObjRender would be passed on to MWRender::RenderingManager, which in turn would call insertObjRender.

btw. I suggest we change the Obj part of the name into Object. No point in having cryptic abbreviations.
swick wrote: I've collected all inforamation and it would be nice if you can confirm that everything is right or anything is not in the list or anything is just wrong.

MWRender::RenderingManager
- manages instances of the Scene class and the middle-layer classes
- should not do anything but forward calls to its members.
- addCell (CellStore *store);
- removeCell (CellStore *store);
- addObject (const MWWorld::Ptr& ptr, CellStore *store);
- removeObject (const MWWorld::Ptr& ptr, CellStore *store);
- moveObject (const MWWorld::Ptr& ptr, const Ogre::Vector3& position);
- scaleObject (const MWWorld::Ptr& ptr, const Ogre::Vector3& scale);
- rotateObject (const MWWorld::Ptr& ptr, const::Ogre::Quaternion& orientation);
- moveObjectToCell (const MWWorld::Ptr& ptr, const Ogre::Vector3& position, CellStore *store);
- setPhysicsDebugRendering (bool);
- getPhysicsDebugRendering() const;
- update (float duration);
- getPlayerOrientation
- void playAnimation (Ptr ptr, const std::string& animation, bool finishCurrentAnimation, bool skipToLoop, int loopCount = 1)
- void skipAnimation (Ptr ptr)
- some functions for controlling player camera mode
- all of the sky-related functions

MWRender::Objects
- object management
- batching
- must to be able to put a whole cell into a scene efficiently (and also remove it)

MWRender::RenderingInterface
- replaces CellRenderImp

MWRender::Creature : MWRender::RenderingInterface

MWRender::NPC : MWRender::RenderingInterface


MWRender::Player : MWRender::RenderingInterface
- The MWRender::RenderingManager class should filter out Player-related calls and redirect them to the MWRender::Player instance.
- handles camera modes and camera positioning (we don't need to implement them during the refactoring; this is a separate task)
- in first person mode, the player scene node should simply be made invisible

MWRender::Sky
- old sky manager
- drop the PIMPL idiom (not needed here and inconsistent with the rest of OpenMW)
MWRender::Water
- dummy

MWRender::Terrain
- dummy

MWRender::Debugging
- various pieces of special rendering, that is used during content debugging; e.g. physics collision shapes

MWRender::Scene
- interface to OEngine::Render::OgreRnederer
- needs to handle the root node (see old MWScene class)
- remove physiks

MWWorld::PhysiksSystem
- physiks
- vector full of handle/coordinate pairs
- not depend on MWWorld::World

MWWorld::World
- instace of MWRender::RenderingManager
- remove SkyManager
- remove mBufferedCells
- mActiveCells must be changed from a map to a set

MWWorld::Class
- insertObjectRender
- insertObjectPhysic
- insertObjectMechanics

MWWorld::Scene
- iterates and call MWWorld::Class insertObjectRender and insertObjectPhysic
Zini wrote: Several problems:

1. We decided to completely drop MWRender::Scene. What little functionality this class had, can be folded into OpenEngine.

2. I think we need to remove MWRender::RenderingManager.:addCell. Adding cells is handled by the MWWorld::Scene class (via MWRender::RenderingManager::addObject). That means at construction MWRender::RenderingManager should receive a reference to MWWorld::Scene, since it needs to query it in some situations about which cells are active.

3. With the new design we can partially drop the requirement for MWRender::Objects regarding efficient adding of whole cells. Removing should still be handled efficiently thought. For the batching keep in mind, that objects can be changed all the time, but the batching should only be updated at the end of an update-cycle. I guess we should add a

void updatesFinished()

function to Objects. It can be called by MWRender::RenderingManager::update, if you make sure the frame listener in Engine is called this function last (after all changes for this frame have been made).

4. MWRender::RenderingManager should not only filter out calls to Player, but also to Creature and NPC.

5. mActiveCells will be handled by MWWorld::Scene instead of MWWorld::World from now on.

6. MWWorld::Scene must manage a list of active cells (see above) and both MWWorld::World and MWRender::RenderingManager must be able to query it.

7. When adding a cell (or an object) MWWorld::Scene must call MWWorld::Class::insertObjectPhysic and MWworld::Class::insertObjectMechanics, but not insertObjectRender. Instead MWRender::RenderingManager::addObject should be called, which will call insertObjectRender with the right RenderingInterface instance (Objects, Npc, Creature or Player) as parameter.

8. I think it has not been decided yet, if we need both a NPC class and a Creature class or if they can both be merged into an Actor class. I interpret jhook's latest statement in favour of the actor class, but a final clarification would be welcome.

9. Objects must be derived from RenderingInterface too.
Zini wrote: Forgot to mention one thing: It would be helpful, if you can start with the MWWorld::Scene class. I need these changes for my own task for 0.12.0.

Actually, it might be a good idea to split the refactoring task in three parts:

1. MWWorld::Scene
2. MWWorld::PhysiksSystem
3. MWRender

Each part should be fairly self contained. Should be easier than doing the whole huge task in one go.

Edit: It might make sense to swap 1. and 2. Not entirely sure. If it does, don't worry about my task. It will take a while before I get to it anyway.
swick wrote: Just one question more: On which branch I should work?
Zini wrote: Start with my next branch and create a new feature branch (mwrender or something) from it.
swick wrote: Somehow, I got stuck. The programm is allways receiving a segmentation fault signal when I try to forward World::changeToInteriorCell to Scene::changeToInteriorCell.

https://github.com/swick/openmw/tree/mw ... mw/mwworld
Zini wrote: You are not creating a Scene object as far as I can tell. The mWorldScene pointer is pointing into nirvana.

Edit: And wto minor corrections while we are at it. Please don't use underscores in filenames (the coding style guide in the wiki says so .. oh, well) and don't start include guard preprocessor names with an underscore (technically you are not allowed to use a name starting with an underscore followed by an uppercase letter).
swick wrote: right, but it's not the problem.

Code: Select all

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff5876233 in std::basic_ostream<char, std::char_traits<char> >& std::operator<< <char, std::char_traits<char>, std::allocator<char> >(std::basic_ostream<char, std::char_traits<char> >&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
   from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
(gdb) bt
#0  0x00007ffff5876233 in std::basic_ostream<char, std::char_traits<char> >& std::operator<< <char, std::char_traits<char>, std::allocator<char> >(std::basic_ostream<char, std::char_traits<char> >&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
   from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#1  0x000000000098ee0e in MWRender::Player::getHandle() const ()
#2  0x000000000098e6d8 in MWWorld::Player::Player(MWRender::Player*, ESM::NPC const*, MWWorld::World&) ()
#3  0x0000000000929344 in MWWorld::World::World(OEngine::Render::OgreRenderer&, OEngine::Physic::PhysicEngine*, Files::Collections const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, boost::filesystem3::path const&, bool, MWWorld::Environment&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
#4  0x000000000088ae2d in OMW::Engine::go() ()
#5  0x0000000000881a18 in main ()
Seems like the problem is in MWRender::Player but i didn't change it
Zini wrote: It looks like there is no valid MWRender::Player object. But I don't know why. You must have screwed something up during the initialisation.

Other problems I see (unrelated to the crash):

- you are not cleaning up mWorldScene

- you are moving a lot of code into scene, that does not belong there, e.g. getPtr, enable, disable, deleteObject and moveObject (not sure if the list is complete). These functions work on reference no matter if they are in the OGRE scene or not.
swick wrote: Making progress, all cell related things are now in World::Scene but also some mMechanicsManager and mSoundManager (https://github.com/swick/openmw/blob/mw ... ne.cpp#L35) should i remove them somehow? And pls check if everything else is right...
Zini wrote: From reading the headers and quickly glancing over the source files, I find the following:

There are still things in the Scene that belong to the World. These are:

mInteriors, mExteriors: Cells exist no matter if the player is in them or not.

The getExteriors function is definitely part of the world class.


The mechanics and sound calls are okay. These are informing the two respective subsystems about things going in or out of active cells. These calls belong into the scene class.
swick wrote: Having trouble with a vector but the error doesnt help me:
https://gist.github.com/1120370
Zini wrote: This:

Code: Select all

std::vector< std::pair<const std::string&, btVector3> > response;
definitely looks wrong. You can't put a reference into a STL container.

btw. I see you are using the btVector3 type in the interface. Is that an internal helper function? If not, then change it please. The physics class should not expose Bullet to the outside world.
swick wrote:
It would be better to have the phyiscs system generate a vector full of handle/coordinate pairs
Or what do oyu mean with coordinate pairs?
Zini wrote: I assume you are quoting a statement from me. Don't remember in what context I was making it. But from the phrasing the meaning is pretty obvious. Handle means an Ogre-handle, which is a std::string. As vector class (coordinates) we use Ogre::Vector3 in most cases. And the pair is std::pair.
swick wrote: When I'm right it should look like this:
https://github.com/swick/openmw/commit/ ... e01f8ef294
Zini wrote: Please stop this string micro-optimisation. It's std::string. Not const std::string * and not const std:::string &. Just std::string. Anything else is most likely unneeded and may get you in trouble.

The rest looks okay-ish.

I think we should improve the moveObject call a bit more. MWWorld::World::moveObject is changing the position of the object in the world model, the position where the object is rendered in the OGRE scene and the position in the physics system.
The last function is disabled during physics update, but we can do that more clearly now

Create a new function World::moveObjectImp, which does all what moveObject does now minus the physics part. Then change moveObject to a call to moveObjectImp plus the physics part.
swick wrote: done https://github.com/swick/openmw/commit/ ... a7470001b8

There is a segfault on shutdown (since a few commits) but I'm searching the nasty bug...
Zini wrote: moveObjectImp must be private, since it does an incomplete move and calling it is only valid from the physics update function and from moveObject. Exposing it to the outside would might break class invariants.
gus wrote: Wow swik, you are working really fast!
Edit: Do you have at least enough time to have a go at: http://bugs.openmw.org/issues/154 ?

This is an important task and I don't think anyone else will pick it up any time soon.
I will try.
swick wrote: I've to ask a few more questions:
You want to drop MWRender::CellRender, MWRender::CellRenderImp, MWRender::ExteriorCellRender and MWRender::InteriorCellRender?
If it's true, where do you want to put the whole insertMesh, rotateMesh etc. (OpenEngine?) ?
When I cant access insertMesh etc. how should I test the code?
edit: And how should the MWWorld::Class subclasses call the mesh things?
Why is there no insertObject in MWWorld::World?

Sry for asking so much (stupid) questions but never did something like this before...
Zini wrote:
You want to drop MWRender::CellRender, MWRender::CellRenderImp, MWRender::ExteriorCellRender and MWRender::InteriorCellRender?
Correct.
If it's true, where do you want to put the whole insertMesh, rotateMesh etc. (OpenEngine?) ?
MWRender::RenderingInterface. With implementations in the derived classes. Derived classes, that can't implement a function meaningfully, should throw an exception when the function is called. I suggest you leave out all the NPC/Creature stuff for now. Jhooks can add it later.

btw. I suggest to drop all the mesh manipulation functions for existing render elements and to replace them with general manipulation functions (e.g. rotate instead of rotateMesh). and you don't need the overloaded versions of the insertMesh function either.
When I cant access insertMesh etc. how should I test the code?
Don't know what you mean here.
And how should the MWWorld::Class subclasses call the mesh things?
Why is there no insertObject in MWWorld::World?
The insertObjectRender of MWWorld::Class takes a reference to RenderingInterface as an argument.
Sry for asking so much (stupid) questions but never did something like this before...
No problem and your questions aren't stupid. This is a very complex design.
swick wrote: MWRender::ExteriorCellRender and MWRender:InteriorCellRender are realy much the same (only difference is the isStatic flag in ExteriorCellRender). When I'm right this should be the MWRender::RenderingInterface in the new version.
So, when calling MWRender::RenderingManager::addObject it will look up, which type of object it is, then search the MWRender::RenderingInterface subclass (e.g. MWRender::Player, MWRender::Actors). Then it will pass the MWRender::RenderingInterface to the MWClass::Class subclass.

MWRender::RenderingManager::addCell should just iterate over all objects in the cell like CellRenderImp::insertCell is doing atm?
Zini wrote:
MWRender::ExteriorCellRender and MWRender:InteriorCellRender are realy much the same (only difference is the isStatic flag in ExteriorCellRender). When I'm right this should be the MWRender::RenderingInterface in the new version.
So, when calling MWRender::RenderingManager::addObject it will look up, which type of object it is, then search the MWRender::RenderingInterface subclass (e.g. MWRender::Player, MWRender::Actors). Then it will pass the MWRender::RenderingInterface to the MWClass::Class subclass.
Correct, except for the isStatic flag only being in exterior. If it is, than that is a bug. It should also be in the interior.
MWRender::RenderingManager::addCell should just iterate over all objects in the cell like CellRenderImp::insertCell is doing atm?
Correct for object management. But addCell also needs to keep track which cells are currently rendered.
Zini wrote: btw. how is that crash bug you mentioned coming along? I just finished a larger chunk of work for the Redemption project today, so I should be able to put a bit of time into my task for 0.12.0 next week. It will be based on your code changes and therefore they need to be made working properly first.
swick wrote: in World::~World
delete mWorldScene;

Code: Select all

glibc detected *** /home/sebastian/prog/OpenMW/Build/openmw: free(): invalid pointer: 0x0000000003433c78
Dont know why. I dont delete or free it anywhere else.
Zini wrote: I don't get your crash which makes it very hard to help you with the debugging. On the other hand I get tons of error messages:
Error in framelistener: unknown Ogre handle: Unnamed_1539
(this happened in Balmora)
swick wrote: i just removed the line. The other errors only appear in exterior cells and i cant test them (0fps problem).
Zini wrote: We need to stop here with the refactoring and sort out these two bugs. It will only get harder to pin them down when more refactoring has been done.
Zini wrote: Found your crash bug (fix pushed to github). You forgot to make your mScene member variable in the MWorld::Scene class a reference, which resulted in the MWRender::MWScene instance being copied. Which isn't a operation that MWScene supports and unfortunately it isn't disabled either. Just another one of these pitfalls we inherited with the old code base. I truly hate the MWScene class. Headache right from the beginning and worst piece of design ever.

Anyway, now we only have the second bug left. I suspect it is something in the physics system, but right now I have no idea what to look for.
swick wrote: I'll merge that in, thx. I think that now I can't chage anything without breaking the whole thing, so for the next commits you won't be able to start openmw anymore. Is that okay for you?
Zini wrote: Having a few commits that don't run is okay. But there is still one bug left, that needs to be fixed first.
Locked