Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dirty flag and integrate #1725

Merged
merged 32 commits into from
Jan 17, 2025
Merged

Conversation

dmwever
Copy link
Contributor

@dmwever dmwever commented Dec 4, 2024

fixes #1679. First time working with c++. Been struggling to update a test and getting it to build. Any advice will be helpful. I am using Visual Studio Code.

Please offer much critique. Complain with vigor. Correct with fervor.

@dmwever
Copy link
Contributor Author

dmwever commented Dec 4, 2024

A few questions I have for this:
First, how to I integrate the clock with the Map/Terrain/Pathfinding subsystem? I don't just want to start passing the clock through everything, but it seems like, with this task, as well as the Cost Stamp and Record History issues, time will be needed in the pathfinding system. How will this be integrated?

Also, would it be good practice to include non-time based overrides for CostField set_cost which simply set the time to TIME_ZERO for initialization? I have done this for the CostField function that is used by the Map class, but only to avoid needing to pass in a time_t from the map.

Lastly, how do I run with debugging using Visual Studio Code? I make changes to a file and run, but it ignores my breakpoints.

libopenage/pathfinding/cost_field.cpp Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.cpp Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.cpp Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.cpp Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/path.h Outdated Show resolved Hide resolved
libopenage/pathfinding/integrator.h Outdated Show resolved Hide resolved
libopenage/pathfinding/integrator.h Outdated Show resolved Hide resolved
libopenage/pathfinding/sector.h Outdated Show resolved Hide resolved
libopenage/pathfinding/sector.h Outdated Show resolved Hide resolved
@heinezen
Copy link
Member

heinezen commented Dec 5, 2024

First, how to I integrate the clock with the Map/Terrain/Pathfinding subsystem? I don't just want to start passing the clock through everything, but it seems like, with this task, as well as the Cost Stamp and Record History issues, time will be needed in the pathfinding system. How will this be integrated?

Where would you need to pass the clock? You very likely only need to pass the time value but the clock is not necessary IMO.

Also, would it be good practice to include non-time based overrides for CostField set_cost which simply set the time to TIME_ZERO for initialization? I have done this for the CostField function that is used by the Map class, but only to avoid needing to pass in a time_t from the map.

That would kind of counteract introducing dirty flags, so I'm not sure if this is a good idea. In fact, I'm not really sure why we should just always pass a time. Initialization does not always happen at time zero.

Lastly, how do I run with debugging using Visual Studio Code? I make changes to a file and run, but it ignores my breakpoints.

It should just work out of the box. What is your start configuration and output?

@dmwever
Copy link
Contributor Author

dmwever commented Dec 6, 2024

First, how to I integrate the clock with the Map/Terrain/Pathfinding subsystem? I don't just want to start passing the clock through everything, but it seems like, with this task, as well as the Cost Stamp and Record History issues, time will be needed in the pathfinding system. How will this be integrated?

Where would you need to pass the clock? You very likely only need to pass the time value but the clock is not necessary IMO.

Perhaps that's a better question. How to get the time (or any time) into the Map subsystem.

@heinezen
Copy link
Member

heinezen commented Dec 7, 2024

Perhaps that's a better question. How to get the time (or any time) into the Map subsystem.

The update time comes from events in the event system that will trigger a map update. However, I wouldn't worry about that right now, since it is not implemented yet.

@dmwever
Copy link
Contributor Author

dmwever commented Dec 7, 2024

It should just work out of the box. What is your start configuration and output?

If you are talking about the configurations in launch.json, it says:
"configurations": []

That's probably my problem lol. What should it say?

@heinezen
Copy link
Member

heinezen commented Dec 7, 2024

Something like this should work as a configuration:

{
    "name": "Main",
    "type": "cppdbg",
    "request": "launch",
    "cwd": "${workspaceFolder}/bin",
    "program": "${workspaceFolder}/bin/run",
    "args": [
        "main",
        "--verbose",
        "--gl-debug",
    ],
    "setupCommands": [
        {
            "description": "Enable pretty-printing for gdb",
            "text": "-enable-pretty-printing",
            "ignoreFailures": true
        },
        {
            "text": "source ${workspaceFolder}/etc/openage.gdbinit"
        }
    ],
}

@dmwever
Copy link
Contributor Author

dmwever commented Dec 7, 2024

Something like this should work as a configuration:

{
    "name": "Main",
    "type": "cppdbg",
    "request": "launch",
    "cwd": "${workspaceFolder}/bin",
    "program": "${workspaceFolder}/bin/run",
    "args": [
        "main",
        "--verbose",
        "--gl-debug",
    ],
    "setupCommands": [
        {
            "description": "Enable pretty-printing for gdb",
            "text": "-enable-pretty-printing",
            "ignoreFailures": true
        },
        {
            "text": "source ${workspaceFolder}/etc/openage.gdbinit"
        }
    ],
}

So to clarify, I should put this in launch.json? I am used to Visual Studio with C#.

@heinezen
Copy link
Member

heinezen commented Dec 7, 2024

You should put it in the configurations array, yes.

@dmwever
Copy link
Contributor Author

dmwever commented Dec 7, 2024

You should put it in the configurations array, yes.

[workspace]/bin/run does not exist

@heinezen
Copy link
Member

heinezen commented Dec 7, 2024

Well, you need to build the project before running :D

@heinezen
Copy link
Member

heinezen commented Dec 7, 2024

https://github.com/SFTtech/openage/blob/master/doc/building.md has info on the build procedure.

libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/integrator.cpp Outdated Show resolved Hide resolved
libopenage/pathfinding/integrator.cpp Outdated Show resolved Hide resolved
@heinezen heinezen added improvement Enhancement of an existing component lang: c++ Done in C++ code area: simulation Involved in the game mechanics and simulation labels Dec 15, 2024
@dmwever
Copy link
Contributor Author

dmwever commented Dec 21, 2024

https://github.com/SFTtech/openage/blob/master/doc/building.md has info on the build procedure.

I can't seem to get this process working on windows. I can run using python openage, following the info on this page:
https://github.com/SFTtech/openage/blob/master/doc/build_instructions/windows_msvc.md

But I don't see a bin folder built in openage following the Windows instructions.

libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/cost_field.h Outdated Show resolved Hide resolved
libopenage/pathfinding/demo/demo_1.cpp Outdated Show resolved Hide resolved
libopenage/pathfinding/integrator.h Outdated Show resolved Hide resolved
libopenage/pathfinding/integrator.h Outdated Show resolved Hide resolved
@dmwever dmwever marked this pull request as ready for review January 6, 2025 16:30
@dmwever
Copy link
Contributor Author

dmwever commented Jan 6, 2025

Marking as ready for review, as most of the ground work is laid. There is a seg fault on demo 1 I cannot track using breakpoints, because I cannot get debugging to work with the demo tests using the Windows Build Instructions page. I assume there is a simple error I am making with c++ unordered maps, but I cannot use breakpoint tools at the moment.

The seg fault occurs when accessing the Field Cache in the is_cached method. Aside from that, we are looking pretty good here.

@jere8184
Copy link
Contributor

jere8184 commented Jan 7, 2025

because I cannot get debugging to work with the demo tests using the

Building via

Marking as ready for review, as most of the ground work is laid. There is a seg fault on demo 1 I cannot track using breakpoints, because I cannot get debugging to work with the demo tests using the Windows Build Instructions page. I assume there is a simple error I am making with c++ unordered maps, but I cannot use breakpoint tools at the moment.

The seg fault occurs when accessing the Field Cache in the is_cached method. Aside from that, we are looking pretty good here.

cmake --build . --config RelWithDebInfo should produce debug symbols that you can then provide to your debugger, to allow you to use breakpoints , if you want further assistance feel free to reach out on matrix :)

@heinezen heinezen force-pushed the Add-Cost-Field-Dirty-Flag branch 4 times, most recently from 0300cf5 to b971a7b Compare January 13, 2025 04:10
@heinezen
Copy link
Member

I fixed your segfault and a few other things. The segfault happened because the field cache was not initialized in the Integrator's constructor.

The only thing left to do for you now is to add yourself (and your commit mail) to copying.md and we can merge :)

heinezen
heinezen previously approved these changes Jan 13, 2025
@dmwever
Copy link
Contributor Author

dmwever commented Jan 13, 2025

Added to copying.md!

dmwever and others added 26 commits January 17, 2025 09:47
Co-authored-by: Christoph Heine <[email protected]>
Co-authored-by: Christoph Heine <[email protected]>
It should only be checked from the cost field.
- Make read-only methods const
- Add get(..) method for getting entire field entry to avoid two lookups
- Simplify lookup of entries
- Add more docstrings
@heinezen heinezen force-pushed the Add-Cost-Field-Dirty-Flag branch from 66e4004 to 0645469 Compare January 17, 2025 08:47
@heinezen heinezen enabled auto-merge January 17, 2025 08:49
Copy link
Member

@heinezen heinezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we go!

@heinezen heinezen merged commit 6597b09 into SFTtech:master Jan 17, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: simulation Involved in the game mechanics and simulation improvement Enhancement of an existing component lang: c++ Done in C++ code
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Invalidate flow field cache entry when cost field changes
5 participants