added drawing functionality to tactical maps #3363
Conversation
|
You currently have a negative Fix/Feature pull request delta of -2. Maintainers may close this PR at will. Fixing issues or improving the codebase will improve this score. |
|
drawing not sync'd across maps right? i have minor concerns regarding map export being exploitable by sending any image if it's uploaded back from tgui. it's pretty cool if it can be done at all though |
Toolbar selection is synced but the drawing and the map itself are not I think, so if two users opened up the map and started drawing they would not be able to see the other's map but they would share the color selection which of course would cause problems although this is fixable. I think I put a few comments near the icon2html stating that it wasn't the correct way to do this and I was debugging. |
|
I'm going to be busy for the next week so I won't be able to work on any requested changes until I get back. Ideally, it would be great if someone who is knowledgeable in JS and React/Inferno stuff can take a thorough look before I work on optimizing the access and retrieval of the pngs. Thanks. |
ghost
closed this
ghost
reopened this
ghost
marked this pull request as draft
ghost
marked this pull request as ready for review
There was a problem hiding this comment.
Some parts are looking good. I think it could be greatly simplified if, instead of rebuilding the image on update - to change global state for the lines reference. This would allow the end user to get the line data, have their browser redraw it based upon the global state, and we wouldn't have to keep resending out images.
I feel that doing it this way (assuming recaching isnt an issue) - will result in a new image being sent out via the resource transit system. This will result in a delay between the map being generated and sent to the user, they will have a blank screen for a while.
I might be wrong in my thoughts - but this approach could alleviate the concerns the maintainer team have regarding uploading and forced distribution of images. All it takes is for one person to start doing it and we have a big problem on our hands.
| @@ -472,15 +486,104 @@ SUBSYSTEM_DEF(minimaps) | |||
|
|
|||
| ui = SStgui.try_update_ui(user, src, ui) | |||
| if(!ui) | |||
|
|
|||
| var/icon/map_icon = getFlatIcon(map_holder.map) | |||
| img_ref = icon2html(map_icon, user, sourceonly = TRUE) | |||
There was a problem hiding this comment.
Could this cause a performance issue if say, suddenly, 30-40 marines all request the image at once? I can't remember if repeated calls will recache it.
There was a problem hiding this comment.
my initial commit lul, why ever did I delete this comment
|
This PR has been inactive for long enough to be automatically marked as stale. This means it is at risk of being auto closed in ~ 7 days, please address any outstanding review items and ensure your PR is finished, if these are all true and you are auto-staled anyway, you need to actively ask maintainers if your PR will be merged. Once you have done any of the previous actions then you should request a maintainer remove the stale label on your PR, to reset the stale timer. If you feel no maintainer will respond in that time, you may wish to close this PR youself, while you seek maintainer comment, as you will then be able to reopen the PR yourself |
About the pull request
Added a drawing feature to tactical maps, this is not done by any means and there is still plenty to do. Javascript and TGUI functionality is mostly done however there are a few bugs and the access and retrieval of the images need to be heavily optimized as the current implementation was just getting the javascript part working. Once everything is correctly implemented cic should be able to announce the tactical map with the drawings to the marines.
Explain why it's good for the game
drawing is fun
Testing Photographs and Procedure
Changelog
🆑
add: added drawing functionality to tactical maps
/:cl: