BGE: Add getActionNames() method to KX_Scene.
Needs ReviewPublic

Authored by Quentin Wenger (Matpi) on Jul 17 2015, 1:22 AM.

Details

Summary

This patch adds a new method getActionNames() to the KX_Scene Python class.
Goal is to be able to check what actions currently exist in the scene, it is particularly useful when working with LibLoad.
We could also do it for objects, but then references in ActionManager only take in account currently playing actions.
The naming of the function follows KX_GameObject.getPropertyNames(), but could be changed if needed.
The patch also adds doc as needed.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D1411
doc/python_api/rst/bge_types/bge.types.KX_Scene.rst
189

This is just rewording the method name, which doesn't add any information. Your description of the patch contains much more info, why not include that information in the documentation?

You may also want to include a reference to this new method to the documentation of KX_GameObject.playAction; I was thinking something along the lines of "See <reference here> to obtain a list of the action names that are currently loaded and available."

191

You could describe it as "list of strings" or "list of str objects", so that it's explicit what's in there.

source/gameengine/Ketsji/KX_Scene.cpp
2594

Is a list really the best data structure for this? If the order doesn't matter (and I feel that this is the case), then a set would be better. With a set a query like actionname in scene.getActionNames() will be more efficient.

source/gameengine/Ketsji/KX_Scene.cpp
2594

Campbell prefer tuple (PyTuple_SET_ITEM) because the are not mutable.

Code not tested, but should work.

	PyObject *list = PyTuple_New(act_map.size());

	for (int index = 0; index < act_map.size(); ++index) {
		PyTuple_SET_ITEM(list, index, PyUnicode_FromString(*act_map.getKey(index)));
	}
Quentin Wenger (Matpi) marked 3 inline comments as done.
  • Change list to tuple, add doc.
doc/python_api/rst/bge_types/bge.types.KX_Scene.rst
189

Ok, I add a bit of info.

I also include references to KX_GameObject.playAction, BL_ActionActuator.action and BL_ShapeActionActuator.action.

191

Updated to "tuple of strings".

source/gameengine/Ketsji/KX_Scene.cpp
2594

I also thought that it would be more logical tu use tuple, but I saw that KX_GameObject.getPropertyNames uses list.

I change to tuple for now.

Seems like PyTuple_SetItem is safe in our case (topic on stackoverflow), which one is prefered?

Note: Sets could be good too, and frozensets are available if immutability is needed. What do you think, @Sybren A. Stüvel (sybren), @Thomas Szepe (hg1) and @Campbell Barton (campbellbarton)?

  • Simplify doc references from bge.types.KX_Scene to KX_Scene.
Sybren A. Stüvel (sybren) requested changes to this revision.Jul 18 2015, 10:08 AM

Can you also upload a Blend file that demonstrates this new feature? You'll also need that for the release log wiki page anyway.

source/gameengine/Ketsji/KX_Scene.cpp
2601

From the stackoverflow post you linked, it shows that you can use PyTuple_SETITEM (note the different capitalisation). It's slightly faster because it doesn't do any error checking, which is fine as the tuple is brand new and we're 100% sure about that.

This revision now requires changes to proceed.Jul 18 2015, 10:08 AM
  • Change PyTuple_SetItem to PyTuple_SET_ITEM, should be a bit faster.

For the .blend file: I could make a simple one, but as previously said, the feature is particularly useful for LibLoad.
Are .zip files containing multiple .blend allowed for the release notes?

Quentin Wenger (Matpi) marked an inline comment as done.Jul 18 2015, 6:18 PM

It's PyTuple_SET_ITEM, not PyTuple_SETITEM, btw. ;-)

Why not implemnted a fully python proxy for SCA_LogicManager ?

Why not implemnted a fully python proxy for SCA_LogicManager ?

Good question. I think it depends on how much functionality of SCA_LogicManager is worth exposing to Python as well.

Thomas Szepe (hg1) commandeered this revision.Aug 2 2015, 8:04 PM

For the .blend file: I could make a simple one, but as previously said, the feature is particularly useful for LibLoad.

Actually for me it is not clear why this patch is needed. Because if you want to plan an action by using an action actuator or the playAction() methods, you need to define the action names. So please provide an example file which is showing the use case.

Are .zip files containing multiple .blend allowed for the release notes?

Yes, why not?

The .rst documentation changes in BL_ActionActuator.rst, BL_ShapeActionActuator.rst, KX_GameObject.rst are not necessary and should be removed.
Basically I think the documentation should only describe what the method or attribute is doing and not what other method's are exiting. Except for deprecated methods or attributes.

source/gameengine/Ketsji/KX_Scene.cpp
2595

One space after the data type.
CTR_Map<STR_HashedString, void *> act_map = m_logicmgr->GetActionMap();

@Quentin Wenger (Matpi). I accidentally selected commandeered revision. So the author has changed. I don't found a way to change it back. So please check if you can commandeered revision back to you.

Quentin Wenger (Matpi) commandeered this revision.Aug 2 2015, 9:07 PM

Thanks for the review, I'll make changes as soon as I can (got no time now, exam weeks at the university).

The .rst documentation changes in BL_ActionActuator.rst, BL_ShapeActionActuator.rst, KX_GameObject.rst are not necessary and should be removed.
Basically I think the documentation should only describe what the method or attribute is doing and not what other method's are exiting. Except for deprecated methods or attributes.

I disagree with this. When several methods and attributes are related, this relation should be documented as wel. It makes documentation more easy to read, and to be honest I don't see a downside.

The .rst documentation changes in BL_ActionActuator.rst, BL_ShapeActionActuator.rst, KX_GameObject.rst are not necessary and should be removed.
Basically I think the documentation should only describe what the method or attribute is doing and not what other method's are exiting. Except for deprecated methods or attributes.

I disagree with this. When several methods and attributes are related, this relation should be documented as wel. It makes documentation more easy to read, and to be honest I don't see a downside.

I don't agree with your point of view.

  1. I think the API documentation should only describe the API it self, like the rest of the blender documentation.
  2. Also it will not be uniform to the rest of the documentation.
  3. If we do this here, we will end up in documenting every function which is related to an other function. Like KX_GameObject.name and KX_Scene.objects or KX_Scene.objectsInactive. Same for lights, cameras, active_camera and so on. Which makes the documentation harder to read (longer documentation more to red more to understand).
  4. Non programmers should use LogicBriks or finished scripts. A programmer should know what he does and how to use the search function in the documentation.
  5. When I use a attribute like "action" on the actuator, I know that I only get this action name. So pointing me to the KX_Scene.getActionNames() is useless. Because when I want to get the currently used action name for the actuator, why I should be interested in getting all actions of the whole scene.
  6. Also for me, getting the the actual setted action name is not really related with a list of all available action names for this scene.

Basically I think when we get starting here doing something like this, we will end up in an over documentation mess with a lot of useless information and links.

I think that the rest of the Blender documentation is also pretty terse, and really could use some extending.
It's an ongoing effort. Of course we can overhaul the entire documentation, but nobody's got time to do that.

I agree that we should find a nice balance between brevity and clarity. However, at a point in a lot of documentation where they say "here you input any x ϵ X", I ask myself "ok, where do I find X?" -- in my opinion, this should be part of the documentation. A programmer will not instantly know all of the API by heart. Having cross-links in the documentation can help tremendously in understanding how things fit together.

When I use a attribute like "action" on the actuator, I know that I only get this action name. So pointing me to the KX_Scene.getActionNames() is useless. Because when I want to get the currently used action name for the actuator, why I should be interested in getting all actions of the whole scene.

Don't forget that it's also a setter. Of course for just a getter this link would be utterly irrelevant, I agree, but when you need to set something by picking a string from a given set of strings, pointing to the location where you can find that given set of valid values seems like a good idea to me.

Resigning as reviewer, as I don't have time for BGE work any more.