GraphicsConfig spritePath() hidden behavior
Thumperrr opened this issue · 7 comments
GraphicsConfig::spritePath() currently appends "chesspp" to the beginning of the specified path.
This kind of hidden behavior can be confusing and counterintuitive.
Although everything in the graphics.json file should be under the "chesspp" node, I don't think we should jump to the conclusion that it's impossible for there to be another top-level node, and have users specify the full path when requesting a resource.
I bring this up because @ResidentBiscuit and I were thrown for a loop for a while when his added resources were producing the "missing" image, when the json path he provided to GraphicsConfig looked correct. Even though I wrote a huge amount of the graphics functionality, I was still at a loss for a while. So that says something.
::chesspp::config::GraphicsConfig is a deriving class from ::chesspp::config::Configuration. We could just add a memember function to Configuration to get the raw NestedValue with any json path you like. However I don't think we should change spritePath as it is specifically for the chesspp namespace - we would use a GraphicsConfig in a different namespace otherwise.
My argument is that it's not a namespace. It's a path inside the json file. If you're looking at the json file, it doesn't make sense to me to have to explicitly leave out "chesspp" when creating paths. I see no reason why there couldnt be another top level node beside chesspp.
That is, spritePath("chesspp", "board", "image") is incorrect in the current code and spritePath("board", "image") is correct.
That initially makes sense but it's another thing to think about when you're looking at the json file. "Oh, I know the path is chesspp/board/image, but I don't need to specify the first part."
If I'm the only one that has this issue then I'll gladly forget about it.
-Stephen Hall
On Dec 24, 2013, at 4:18 PM, "LB--" notifications@github.com wrote:
::chesspp::config::GraphicsConfig is a deriving class from ::chesspp::config::Configuration. We could just add a memember function to Configuration to get the raw NestedValue with any json path you like. However I don't think we should change spritePath as it is specifically for the chesspp namespace - we would use a GraphicsConfig in a different namespace otherwise.
—
Reply to this email directly or view it on GitHub.
I can see both sides here. It did throw me off and seem like unexpected
behavior, though.
On Tue, Dec 24, 2013 at 3:58 PM, Thumperrr notifications@github.com wrote:
My argument is that it's not a namespace. It's a path inside the json
file. If you're looking at the json file, it doesn't make sense to me to
have to explicitly leave out "chesspp" when creating paths. I see no reason
why there couldnt be another top level node beside chesspp.That is, spritePath("chesspp", "board", "image") is incorrect in the
current code and spritePath("board", "image") is correct.That initially makes sense but it's another thing to think about when
you're looking at the json file. "Oh, I know the path is
chesspp/board/image, but I don't need to specify the first part."If I'm the only one that has this issue then I'll gladly forget about it.
-Stephen Hall
On Dec 24, 2013, at 4:18 PM, "LB--" notifications@github.com wrote:
::chesspp::config::GraphicsConfig is a deriving class from
::chesspp::config::Configuration. We could just add a memember function to
Configuration to get the raw NestedValue with any json path you like.
However I don't think we should change spritePath as it is specifically for
the chesspp namespace - we would use a GraphicsConfig in a different
namespace otherwise.—
Reply to this email directly or view it on GitHub.—
Reply to this email directly or view it on GitHubhttps://github.com//issues/67#issuecomment-31186064
.
@Thumperrr the thing is, the class in question is mean to be a convenience. In another namespace would would create a similar class with the same convenience. However now that I think of it it would be better if the json file itself were in a chesspp directory than to have chesspp mentioned in the json.
The only problem I have with that is I don't like mixing C++ terminology and concepts in with json terminology and concepts, which are two different beings of their own. When i look at the json file I don't consider "chesspp" a namespace. I just consider it a root node that's the beginning of a path that gets to the node i'm looking for. If that's the case then I should include it in my search path.
I know it's meant to be a convenience to have "chesspp" specified for you. And I would be considering it as such if I didn't just have that little fiasco with ResidentBiscuit trying to figure out what the hell went wrong. Then, looking back on it, I started to feel like we should have to specify "chesspp" should the chesspp node exist at all.
I think we should get rid of the chesspp from the json altogether and put the json in a chesspp subfolder.
I can agree to that. I'll do that quickly and then open a PR.