MapExpressions

[OrbWeaver]

I suggest dividing the external interface to the Model Selector into two functions -- the current one which is used for choosing a NEW model, and displays all of the extra options, and a new one which is used for choosing an EXISTING model, which displays no extra options (and could theoretically not bother expanding skins either).

 

Obviously the two functions would need to return different types, I suggest keeping the ModelCreationSettings (or whatever it is called) for the new model function, and just returning a string model name for the existing model function.

[mohij]

I think that's overtuned.

When it's only about hiding the options entirely a nice

ModelSelectorResult chooseModel(bool showOptions=true);

should do the job cleaner.

[OrbWeaver]

Yes, that would do just as well, provided that it was understood that the options fields in the returned object were meaningless (or just set them to false by default).

[mohij]

But you have to set showOptions to false first, and if you set that one to false, you should already understand that the results in the returned structure are meaningless.

 

I think I will now add the "add clipBrush" option to the right-click-menu.

[greebo]

I would vote for hiding it if the option does not make sense in this context.

 

edit: Whoops, just saw the new posts on the new page.

[mohij]

How can I iterate the SelectionList? The SelectionList is a template (is there more than one?), and the iterator is templated to, so I don't know how to use the iterator. Same problem with a->name. Where is the actual instance of the SelectionList?

	for(iterator a = SelectionList.begin(), a != SelectionList.end(), a++) { 
	 // TODO: to be replaced by inheritance-based class detection 
	 if (!string_equal_nocase(a->name, "func_static")) { 
		 nonModel = true; 
		 break; 
	 } 
 }

[greebo]

The SelectionList is a private member of the SelectionSystem, so you shouldn't be directly iterating over it. The way to traverse the current selection is to call

GlobalSelectionSystem().foreachSelected(Visitor& visitor);

What do you intend to do? Maybe there is already existing functionality, that I know of?

[mohij]

I want the menu button "create MonsterClip" to work when one or more models are selected. So I want to check the SelectionList for non models and switch the button sensible/insensible. When called I'd like to create the monsterClip for every item in the SelectionList.

[greebo]

I see. Best thing is to write a Visitor then:

typedef std::vector<scene::Path> InstanceList;

class ModelFinder :
  public SelectionSystem::Visitor
{
  InstanceList& _targetList;
public:
  ModelFinder(InstanceList& targetList) :
 _targetList(targetList)
  {}

  void visit(scene::Instance& instance) const {
   Entity* entity = Node_getEntity(instance.path().top());
   if (entity != NULL && isModel(entity)) {
		_targetList.push_back(instance.path());
   }
  }
};

// somewhere else in the code
{
InstanceList modelList;
ModelFinder visitor(modelList);
GlobalSelectionSystem().foreachSelected(visitor);

if (modelList.size() > 0) {
   // One or more models found
}
}

I don't know if the above code works, I haven't compiled it. Could be that I forgot to add a "mutable" keyword in the visitor class due to that stupid "const" declaration in the abstract base class.

[mohij]

With that visitor class the "create monsterClip" button would work even with other stuff selected, but only operate on the models. Is that intended?

[greebo]

Ah, not really intended. You're right, this has to be considered of course. A boolean flag set to true in the visitor class or anything else what comes to your mind will do the trick, I guess.

[mohij]

Yeah, that's what I tried for the last half hour.... grrrrr

that stupid const function.

Now I ended up keeping a bool* and an InstanceList* in the class and changing the outside objects....

[greebo]

You can declare your bool as mutable:

class ModuleFinder :
  public SelectionSystem::Visitor
{
  mutable _nonModelFound;
public:
  // bleh
};

This mutable keyword can be used to make members writable even in const member methods. It's of course bad style to use it (in well-designed code it shouldn't be used at all, imo), but I guess it's ok in this case.

 

If you're feeling ambitious, you can of course change the SelectionSystem::Visitor abstract base class and remove the const keyword. I don't see why visitors need to be const while traversing the selection.

[mohij]

I hoped you would say that *happy*

 

Okay, another question: Models are listed under the "func_static" entity. What key do I need to check for that?

[greebo]

You can recognise models like this:

• Instance is an Entity
  
• Entity has spawnarg "classname" == "func_static"
  
• Entity has non-empty spawnarg "name"
  
• Entity has non-empty spawnarg "model" != "name"
  

I think the best thing would be to extend the Entity interface (defined in ientity.h) to include a

virtual bool isModel() const = 0;

method to check whether this entity is a model. If you don't want to dig that deep, I can add this method for you, as I'm very familiar with the various Entity classes.

[mohij]

I'd be gratefull if you do that, since I really don't entirely know the Scenegraph yet. Especially after you said how to recognize models.

An Instance can be an entity? I thought an Instance was a path with rootnode, entity and primitives (nodes).

Non-empty spawnarg name? in ientity.h: STRING_CONSTANT(Name, "Entity");

I obviously get some things wrong here...

[greebo]

Sorry, if I confused you about the Instances. It's easy to get lost in the scenegraph, speaking from my own experience. I only recently wrapped my head about the inner workings and I'm still not entirely enlightened about a few things.

 

The scenegraph consists of scene::Nodes, which build up a tree. In theory, each node can have one or more child nodes. The topmost node is called the scenegraph root. There are several scene::Node classes in DarkRadiant, each with a different set of properties.

• RootNode (represents the scenegraph root)
  
• BrushNode
  
• PatchNode
  
• Doom3GroupNode (the most complex of these all)
  
• EclassModelNode
  
• GenericEntityNode
  
• LightNode
  
• MD5ModelNode
  
• PicoModelNode
  

The Doom3GroupNode, EclassModelNode, GenericEntityNode and LightNode are Entities. They can have spawnargs (key/value pairs). To recognise a node as Entity, call Node_getEntity() or Node_isEntity(). This will try to cast the INodePtr onto an Entity*.

 

An Instance is not a path, but an instance has a path. The path is used to lookup the instance in the scenegraph, it's some sort of unique ID. Every node needs a path before it can be instantiated.

 

(I'm just realizing, that I don't explain that very well here, so hopefully I'm not confusing you even more. Sorry about that. )

 

Forget the bit about STRING_CONSTANT, this is mostly legacy stuff and I'm almost sure we can remove that. It was used to implement a home-grown type-cast system similar to C++'s dynamic_cast.

 

I will implement that function, I'll post back here when it's done.

[greebo]

Ok, the latest SVN revision features the isModel() method. Call it on the Entity:

Entity* entity = Node_getEntity(scene::INodePtr);
if (entity->isModel()) {
  // blah
}

You'll need to recompile almost everything as many other files are depending on the ientity.h header, that's why it took me so long

 

Btw: there is also a wiki page on the DarkRadiant scenegraph, perhaps this helps to clarify things.

[mohij]

I use the wiki actively all the time (otherwise I would by far haven't come as far as I have). According to your explanation the scenegraph is no real node structure with child/parent nodes, but a derivation structure with a base class and derived subclasses. Is that correct?

 

You implemented that quite fast 8-|

I hope the sync won't require many manual merges...

[greebo]

No, the scenegraph is indeed a tree-like structure of nodes. It's the nodes themselves that are deriving from various base classes.

[mohij]

radiant/ui/ortho/OrthoContextMenu.cpp: In member function 'void ui::OrthoContextMenu::checkMonsterClip()':
radiant/ui/ortho/OrthoContextMenu.cpp:154: error: no matching function for call to 'SelectionSystem::foreachSelected(ui::ModelFinder (&)())'
include/iselection.h:143: note: candidates are: virtual void SelectionSystem::foreachSelected(const SelectionSystem::Visitor&) const
radiant/ui/ortho/OrthoContextMenu.cpp:157: error: request for member 'modelList' in 'ui::visitor', which is of non-class type 'ui::ModelFinder ()()'
radiant/ui/ortho/OrthoContextMenu.cpp:157: error: request for member 'onlyModels' in 'ui::visitor', which is of non-class type 'ui::ModelFinder ()()'

I get this error message that says that my visitor is no class.

typedef std::vector<scene::Path> InstanceList;
class ModelFinder : public SelectionSystem::Visitor {
InstanceList targetList;
bool onlyModels;
public:
ModelFinder() :
		onlyModels(true)
{}
void visit(scene::Instance& instance) {
	Entity* entity = Node_getEntity(instance.path().top());
	if (entity->isModel()) {
		targetList.push_back(instance.path());
	}
	else {
		onlyModels = false;
	}}};

But this is obviously a class. For completeness here the lines that cause the error:

ModelFinder visitor();
GlobalSelectionSystem().foreachSelected(visitor);

I have really no idea what the reason is, as this is really just a normal class. Any ideas?

[OrbWeaver]

It needs to be

 

ModelFinder visitor;

 

not

 

ModelFinder visitor();

 

The second version is actually a forward declaration for a function finder which accepts no arguments and returns a ModelFinder.

[mohij]

After changing this Visitor::visitor from const to nonconst, a LOT of dependencies need to be changed too. At some point (after changing a lot of functions from const to non const) gcc spits this out

radiant/brush/BrushVisit.h: In function 'const Functor& Scene_forEachSelectedBrush(const Functor&) [with Functor = BrushForEachFace]':
radiant/brush/BrushVisit.h:162:   instantiated from 'const Functor& Scene_ForEachSelectedBrush_ForEachFace(scene::Graph&, const Functor&) [with Functor = FaceSetShader]'
radiant/brushmanip.cpp:385:   instantiated from here
radiant/brush/BrushVisit.h:25: error: no matching function for call to 'SelectionSystem::foreachSelected(BrushSelectedVisitor<BrushForEachFace>)'
include/iselection.h:143: note: candidates are: virtual void SelectionSystem::foreachSelected(SelectionSystem::Visitor&) const

But these functions seem to comply.

I did a lot trial-and-error on the functions in those files but at some point gcc always spits out a similar message. I start to wonder if it's a good idea to make the Visitor::visit nonconst...

If someone wants to take a look, I uploaded a diff: monsterClipDiff.diff

Some hint what's wrong would be helpful.

[greebo]

At this point I'd rather suggest making the boolean mutable, perhaps I will look at making the visitor non-const later this day.

[mohij]

So shall I revert all my changes in respect to the nonconsting?