Fix relative paths with the Particle Editor #3137
Conversation
Relative paths make the game panic about insecure filenames when they contain '..'; when a custom particle file located outside the particles/ root folder was selected in the editor, and the object was used to 'Open in Particle Editor', the game would crash. The best fix would've been to dodge the need for relative paths entirely, but that would prevent the settings from opening by default in the particles/ folder. This is, I believe, the second best fix.
|
Does this supersede #3102 ? |
|
Ah, I hadn't noticed that PR, my apologies. I've fixed a few more places that had the same bug though, so I suppose it does supersede the other PR. |
|
My changes apparently didn't work, so this is probably better. |
| // realpath() is necessary for particle files outside the particles/ folder | ||
| std::string path = physfsutil::realpath("particles/" + *m_particle_editor_filename); | ||
|
|
||
| static_cast<ParticleEditor*>(screen.get())->open(path); |
There was a problem hiding this comment.
You really need to static cast here? You can store the new ParticleEditor in a variable and use that. In my opinion, that would be more readable. And then you can std::make_unique in the push_screen function
There was a problem hiding this comment.
The static cast was already there. I've copied the code and changed as little as possible so as not to transform the code too much, since I'm not very familiar with the current coding standards and people's preferences. A static cast will be needed either way if screen's type is a pointer to Scene; I can move the static cast on its own line but I can't really avoid it without changing more code. I can't make_unique the screen directly in push_screen because I need to call the open function on the scene before pushing it.
| static_cast<ParticleEditor*>(screen.get())->open("particles/" + *m_particle_editor_filename); | ||
| { | ||
| // realpath() is necessary for particle files outside the particles/ folder | ||
| std::string path = physfsutil::realpath("particles/" + *m_particle_editor_filename); |
There was a problem hiding this comment.
Hey, whatever happened to FileSystem::join? Or std::filesystem::path::operator/? Not the only place where this happens so if you're gonna fix this then do it there aswell
There was a problem hiding this comment.
Similarly to above, I have opted not to change the code more than necessary to keep the changes minimal and avoid going against eventual preferences. The join function also does not normalize the path, so it would be an extra call rather than a replacement.
| @@ -55,11 +56,17 @@ ParticleEditorOpen::~ParticleEditorOpen() | |||
| void | |||
| ParticleEditorOpen::menu_action(MenuItem& item) | |||
| { | |||
| std::string path; | |||
There was a problem hiding this comment.
What? Can't you just make the variable inside the switch case?
There was a problem hiding this comment.
https://stackoverflow.com/questions/92396/why-cant-variables-be-declared-in-a-switch-statement
The coding style documentation does not specify whether to declare variables outside the switch statement or to create a new scope within the case statement. Since I'm used to the former with SDL and I couldn't find any source as to how to format a scope within a case, I put the variable outside the block. I can move it inside and create a scope, if you could provide me with a preferred formatting template.
|
Particles dont seem to load images correctly atm
|
|
when saving a new particle configuration it says "failed to save particle configuration, retrying" and then is successful |
|
these settings, with a tinted texture, alpha being irrelevant, cause the sprite to flicker at its original brightness then return to the tint its at, this weird flickering seems to always happen? |
|
bi2 level spoiler // heres kind of another bug(?) basically im trying to spawn in particles using a particle zone set to spawn, and one set to kill the particles. the particles seem to be spawning anywhere they like, primarily in the spawn region, but still spawning in above it. not too sure whats going on also as a side note, im not sure how this system interacts with camera.ease_scale, i think zooming out 'dilutes' the particles, because if the amount is set at max 500, then you zoom out, there is more space to be filled by those 500 particles. therefore being diluted. this might be noticeable in some scenarios. in this case, a dynamic max. cap might be required? |
|
For the video, can you send the configuration for the particles? If the "Spawn anywhere" (or similarly named) checkbox is checked, the particles will ignore all the zones and particles will spawn randomly inside the camera bounds. For the rest, I'll give it a look whenever I can. |
|
ok so:
also, it seems that if im loading in like a smidge of the particle zone, like a tile then only a fraction of particles will spawn. so this behaviour can be 'patchworked' as in, at least in my own unique case, i can place the particle zone under some terrain (spawn) and the flickering is not seen by the player. but still i think that particle zones should perhaps load off-screen? because the alternative rn is to have the particles set for the entire sector, where its always loaded in, but that has flickering issues and i cant discriminate where i want spawning not to occur spatially. heres the particle file: ive converted it to .txt pls convert to .stcp. |
|
I've reproduced the flickering bug, I'm working on a fix now. @tobbi Would you prefer that I put all the fixes in one PR, or that I keep them separate per PR? |
Sorry, I forgot about this. I think separate PRs for better readability is better. |
Relative paths make the game panic about insecure filenames when they contain '..'; when a custom particle file located outside the particles/ root folder was selected in the editor, and the object was used to 'Open in Particle Editor', the game would crash.
The best fix would've been to dodge the need for relative paths entirely, but that would prevent the settings from opening by default in the particles/ folder. This is, I believe, the second best fix.