Extending the existing game actuator with a screenshot function, to give also non programmers the ability to take screenshots in the BGE.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| source/blender/makesdna/DNA_actuator_types.h | ||
|---|---|---|
| 191 | Do we need a new value or can we just reuse filename as you do for the Python API? | |
| source/blender/makesdna/DNA_actuator_types.h | ||
|---|---|---|
| 191 | We can reuse filename. But as I can remember I have made this extra variable, to get a different property UI text. | |
| source/blender/makesdna/DNA_actuator_types.h | ||
|---|---|---|
| 191 | If we change the property UI text from filename to a more general one like "Path and Filename, use the \"//\" prefix for a relative path" then the text will fit for both actuators. | |
| source/blender/makesdna/DNA_actuator_types.h | ||
|---|---|---|
| 191 | That should work. | |
| source/blender/makesrna/intern/rna_actuator.c | ||
|---|---|---|
| 1738 | Maybe this could be reworded to "The file to use depending on the mode (e.g., the blend file to load or a destination for saving a screenshot). Use the \"//\" prefix for a relative path." I'm not sure what our guidelines are for multi-sentence tooltips, but I find a tooltip that just repeats the name of the field to be of little use. | |
Comment Actions
@Genome36.Via. I have done all requested changes and the path is still mergeable.
Why you changed the status to need revision?
Comment Actions
Strange, i only registered myself and don't touch anything else.
Either way, sorry for the inconvenience.
EDIT: Now that I think of it, "Moguri requested changes to this revision" and it only took effect right after I subscribed ... Meaning I didn't change status :P
| source/gameengine/Ketsji/KX_GameActuator.cpp | ||
|---|---|---|
| 205 | I don't know why we have everywhere this checks. I removed the if statement and made some test if I can cause a crash. Seems to work. I also made some tests with removed if (canvas) statement. It seams that is works also without this statements. | |
| 211 | I don't know which fault it should print out. If the canvas not existing? | |
Comment Actions
Other than the inline comment it looks ok to me.
| source/blender/makesrna/intern/rna_actuator.c | ||
|---|---|---|
| 1740 | insert a space after destination word | |
Comment Actions
I'm okay with this for the most part. I'm not entirely sure on how the warning is done. It seems a little inconsistent with how we do things, but we usually do things with Python exceptions, which might not make sense in this case. The warning also doesn't seem too terribly user friendly (Canvas isn't something that's really mentioned much outside of the source code).
Comment Actions
It seems a little inconsistent with how we do things, but we usually do things with Python exceptions, which might not make sense in this case.
The screenshot actuator will work without Python. So I can't use a Python exception to print out the error message. I found some printf for error messages in the Blender and BGE code. So I thought it is OK to use printf.
The warning also doesn't seem too terribly user friendly (Canvas isn't something that's really mentioned much outside of the source code).
Actually I don't know what else then canvas I should write. I don't found any error message if canvas is not available. Please make a suggestion for the error message.
Comment Actions
It would be nice to get @Campbell Barton (campbellbarton)'s thoughts on the error handling (the user-friendliness of the error message and consistency with other error reporting). If we don't get a review from Campbell in a reasonable time (a week?) then I think this is still okay to commit.
Comment Actions
Changed the error text from "Canvas not available" to "Rasterizer not available".
As taken with moguri on IRC, for most users it is not obvious what is meant by canvas. So we lie here and say rasterizer instead.