-
Notifications
You must be signed in to change notification settings - Fork 25
Fully support Agents.OpenStreetMapSpace at all apps #86
Conversation
@fbanning I'm having a problem. On current state of PR, when trying to run the
So, this is rather cryptic, and I have two questions:
|
@fbanning everything works now but I had to remove the recipes because i didn't want to deal with this cryptic message that can only be understood by someone that already knows how recipes work. Because I remove recipes there is apparently no more the nice display on hover you've made. I'll wait for you to tell me the simple answer here on how to either enable the recipe or the hover infobox. |
I will read through your work on the PR later but for now let me tell you that adding the recipe system has been a very deliberate decision from my side that I've thoroughly thought through before making the PR for it. From all my understanding of how Makie DataInspector works, it is absolutely necessary for the hovering system to work properly without overwriting the tooltips for other plots (e.g. scatter points on the right hand side of If you want a few simple answers from my point of view: Removing recipes will
I'm currently strongly against this step but want to hold back on a final judgment until after I've read what you've done in this PR. |
perfect, we keep the recipes! I just don't know how to fix them! |
I'll look at your code here to see where the problem was. I maybe have some time later in the afternoon. Will get back to you. |
@fbanning one has to simply replace the new lines I wrote: if user_used_polygons(abmstepper.am, abmstepper.markers)
poly!(ax, abmstepper.markers; color = abmstepper.ac, scatterkwargs...)
else
scatter!(ax, abmstepper.pos;
color = abmstepper.colors, marker = abmstepper.markers,
markersize = abmstepper.sizes, scatterkwargs...
)
end in line 118 of |
Alright, I've finally found some time to look at what you did here. There's a bit of "noise" in the PR (like renaming files) but the ideas in there are certainly good and helpful. Your initiative here caught me a bit off guard and I first needed to understand what your approach was before responding in any way. Anyways, I have now refactored the recipes to properly dispatch on
Once that works, I will make a PR so that we can move on with implementing Would you be OK with me taking some of the more structural changes from your PR here (like renaming files and updating the examples and such) and adding them on top of my working branch? This way we will already have a good starting point for adding |
Hi @fbanning ,
I am assuming you have done this locally?
Please wait. Back in the day when we had Plots.jl plotting for graphspace, our plotting was different there. The way it worked is that the functions
That sounds like an inefficient way forwards. This PR works with three spaces. It actually doesn't work with OpenStreetMap because it is not registered, but it would if it was. What we should do is merge this PR that works perfectly and you branch after the merged PR. The main recipe file I haven't really touched anyways. |
No, on a branch of my fork.
That's how I wanted to do it in the
To be honest, I'm not really happy about this because I feel my concerns are ignored. But since you already merged it, I will look at what can be done with the new state of the master branch. Will take me some time again to understand it and refactor things appropriately. Will open a PR once that's done.
I'm aware of this. You already know that I will register the package once it provides a minimal working state with a few more key features implemented (like colouring ways by their type). This is heavily WIP. I cannot work any faster on it than I'm already doing right now. |
I'm confused now. So far I thought that your concerns are that I've removed the recipes, and this has the negative implication of not showing tooltips. But in any case, I certainly acknowledged this concern:
This fix should be done in a separate PR, for two reasons: (1) the code in this pr is working. I've spent a lot of time making sure that all examples in both the AgentsExampleZoo, the Agents.jl main repo and the If you think there is better action moving forwards besides merging this and starting a new PR from master, then certainly let me know! One thing I can say for sure is that my For the GraphSpace, we are completely on the same page as far as I have understood.
??? This seems to be coming off totally the wrong way. I'm simply stating the fact of what is happening, as if anyone actually tried to actually plot the OSM versions, it wouldn't actually work. Although, seeing your response, it feels like you are putting a lot of artificial strain on yourself about what registering a package means. It really, really, isn't a big deal, nor you need a polished final product. I'm not telling this so that you "work faster", so please don't take it the wrong way. Of course, you are the one who decides in the end, I'm just saying this so that you don't have to be so stressed about putting work into OSMMakie.jl! |
No, it's one way or the other. I just thought it might be easier to do it the other way around but it's OK now. I'm slowly going over the new code on master branch and will copy over everything from my WIP branch that I deem important. Don't worry. It will just take some time. I've opened a draft PR #87 for you to see how it's going on.
Certainly.
Yeah, maybe. |
Currently WIP. Todos:
Main.
preface for OSMMakie once the package is releasedResult: