search

google / earthenterprise

Google Earth Enterprise - Open Source
Apache License 2.0
2.67k stars 889 forks source link

Add IsUpToDate function for SharedStrings #1842

Closed tst-lsavoie closed 4 years ago

tst-lsavoie commented 4 years ago

Fusion determines if an asset is up to date by checking the value of its parameters with the value of the previous version's parameters. Several IsUpToDate functions are defined in IsUpToDate.h, but there is not one for SharedStrings. Most likely SharedStrings are being converted to std::strings and using that IsUpToDate function. However, if we create an IsUpToDate specifically for shared strings, we can eliminate the need to convert to std::strings and use the more efficient SharedString comparison. This should improve Fusion's speed.

Most likely this can be accomplished by simply adding

DEFINE_ISUPTODATE(SharedString);

in IsUpToDate.h (and possibly adding appropriate header files. Alternatively, the SharedString IsUpToDate function can go into SharedString.h.

lacrosse91 commented 4 years ago

Hello, @tst-lsavoie . I would like to work on this. Is it the only file that require the change?

Also anything else that i need to do to test this?

tst-lsavoie commented 4 years ago

Yes, that's the only file that needs to change. It would be good to also have some unit tests for the new function. For extra credit you could write unit tests for all of the functions in IsUpToDate.h.

Other than that, it would be good to run a test where you build a database, add more imagery, and build the database again, then make sure that the database built correctly both times.

Since it's a performance change it would be nice to have some performance numbers showing how much difference it makes in the speed and memory usage of a build. That would require building a project with lots of images. If you don't have the set-up to do that I can take care of it.

Thanks for the help.

lacrosse91 commented 4 years ago

Thanks for reply. I appreciate it very much.

I'm a beginner, so I will try to just add the function.

ravindrabhargava commented 4 years ago

Hi, if no objection and still open, would like to work on it?

tst-lsavoie commented 4 years ago

Hi @ravindrabhargava, thanks for the interest but there's already a PR open for this issue. You are welcome to look through our backlog and see if there are other issues you would like to work on, though.

tst-lsavoie commented 4 years ago

Closed via PR #1843